diff options
Diffstat (limited to 'chromium/components/sync_bookmarks')
19 files changed, 839 insertions, 102 deletions
diff --git a/chromium/components/sync_bookmarks/PRESUBMIT.py b/chromium/components/sync_bookmarks/PRESUBMIT.py index 44c9101cabe..ef269b0d3fa 100644 --- a/chromium/components/sync_bookmarks/PRESUBMIT.py +++ b/chromium/components/sync_bookmarks/PRESUBMIT.py @@ -13,16 +13,11 @@ import re SYNC_BOOKMARKS_SOURCE_FILES = ( r'^components[\\/]sync_bookmarks[\\/].*\.(cc|h)$',) -# The wrapper around lint that is called below disables a set of filters if the -# passed filter evaluates to false. Pass a junk filter to avoid this behavior. -LINT_FILTERS = ['+fake/filter'] - def CheckChangeLintsClean(input_api, output_api): source_filter = lambda x: input_api.FilterSourceFile( x, white_list=SYNC_BOOKMARKS_SOURCE_FILES, black_list=None) return input_api.canned_checks.CheckChangeLintsClean( - input_api, output_api, source_filter, lint_filters=LINT_FILTERS, - verbose_level=1) + input_api, output_api, source_filter, lint_filters=[], verbose_level=1) def CheckChanges(input_api, output_api): results = [] diff --git a/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc b/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc index 2e6d8348cc5..2fd92ff645a 100644 --- a/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc +++ b/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc @@ -56,7 +56,9 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests( !node->is_permanent_node() && base::FeatureList::IsEnabled( switches::kSyncDoNotCommitBookmarksWithoutFavicon)) { - // Force the favicon to be loaded. + // Force the favicon to be loaded. The worker will be nudged for commit + // in BookmarkModelObserverImpl::BookmarkNodeFaviconChanged() once + // favicon is loaded. bookmark_model_->GetFavicon(node); continue; } diff --git a/chromium/components/sync_bookmarks/bookmark_model_merger.cc b/chromium/components/sync_bookmarks/bookmark_model_merger.cc index 97ffc991fb6..a3f4e476986 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_merger.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_merger.cc @@ -455,6 +455,13 @@ void BookmarkModelMerger::Merge() { MergeSubtree(/*local_subtree_root=*/permanent_folder, /*remote_node=*/tree_tag_and_root.second); } + + if (base::FeatureList::IsEnabled(switches::kSyncReuploadBookmarkFullTitles)) { + // When the reupload feature is enabled, all new empty trackers are + // automatically reuploaded (since there are no entities to reupload). This + // is used to disable reupload after initial merge. + bookmark_tracker_->SetBookmarksFullTitleReuploaded(); + } } // static @@ -586,8 +593,9 @@ void BookmarkModelMerger::MergeSubtree( remote_node.response_version(), remote_update_entity.creation_time, remote_update_entity.unique_position, remote_update_entity.specifics); if (!local_subtree_root->is_permanent_node() && - IsFullTitleReuploadNeeded(remote_update_entity.specifics.bookmark())) { + IsBookmarkEntityReuploadNeeded(remote_update_entity)) { bookmark_tracker_->IncrementSequenceNumber(entity); + ++valid_updates_without_full_title_; } // If there are remote child updates, try to match them. @@ -738,8 +746,9 @@ void BookmarkModelMerger::ProcessRemoteCreation( bookmark_node, remote_update_entity.id, remote_node.response_version(), remote_update_entity.creation_time, remote_update_entity.unique_position, specifics); - if (IsFullTitleReuploadNeeded(specifics.bookmark())) { + if (IsBookmarkEntityReuploadNeeded(remote_node.entity())) { bookmark_tracker_->IncrementSequenceNumber(entity); + ++valid_updates_without_full_title_; } // Recursively, match by GUID or, if not possible, create local node for all diff --git a/chromium/components/sync_bookmarks/bookmark_model_merger.h b/chromium/components/sync_bookmarks/bookmark_model_merger.h index 054ad418652..e385a996f6f 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_merger.h +++ b/chromium/components/sync_bookmarks/bookmark_model_merger.h @@ -51,6 +51,10 @@ class BookmarkModelMerger { // and metadata entities in the injected tracker. void Merge(); + size_t valid_updates_without_full_title_for_uma() const { + return valid_updates_without_full_title_; + } + private: // Internal representation of a remote tree, composed of nodes. class RemoteTreeNode final { @@ -198,6 +202,8 @@ class BookmarkModelMerger { const RemoteForest remote_forest_; std::unordered_map<std::string, GuidMatch> guid_to_match_map_; + size_t valid_updates_without_full_title_ = 0; + DISALLOW_COPY_AND_ASSIGN(BookmarkModelMerger); }; diff --git a/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc b/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc index 0622d97f348..b9f8e6be842 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc @@ -72,6 +72,61 @@ MATCHER_P2(ElementRawPointersAre, expected_raw_ptr0, expected_raw_ptr1, "") { return arg[0].get() == expected_raw_ptr0 && arg[1].get() == expected_raw_ptr1; } +class UpdateResponseDataBuilder { + public: + UpdateResponseDataBuilder(const std::string& server_id, + const std::string& parent_id, + const std::string& title, + const syncer::UniquePosition& unique_position) { + data_.id = server_id; + data_.parent_id = parent_id; + data_.unique_position = unique_position.ToProto(); + data_.is_folder = true; + + sync_pb::BookmarkSpecifics* bookmark_specifics = + data_.specifics.mutable_bookmark(); + bookmark_specifics->set_legacy_canonicalized_title(title); + bookmark_specifics->set_full_title(title); + + SetGuid(base::GenerateGUID()); + } + + UpdateResponseDataBuilder& SetUrl(const GURL& url) { + data_.is_folder = false; + data_.specifics.mutable_bookmark()->set_url(url.spec()); + return *this; + } + + UpdateResponseDataBuilder& SetLegacyTitleOnly() { + data_.specifics.mutable_bookmark()->clear_full_title(); + return *this; + } + + UpdateResponseDataBuilder& SetFavicon(const GURL& favicon_url, + const std::string& favicon_data) { + data_.specifics.mutable_bookmark()->set_icon_url(favicon_url.spec()); + data_.specifics.mutable_bookmark()->set_favicon(favicon_data); + return *this; + } + + UpdateResponseDataBuilder& SetGuid(const std::string& guid) { + data_.originator_client_item_id = guid; + data_.specifics.mutable_bookmark()->set_guid(guid); + return *this; + } + + syncer::UpdateResponseData Build() { + syncer::UpdateResponseData response_data; + response_data.entity = std::move(data_); + // Similar to what's done in the loopback_server. + response_data.response_version = 0; + return response_data; + } + + private: + syncer::EntityData data_; +}; + syncer::UpdateResponseData CreateUpdateResponseData( const std::string& server_id, const std::string& parent_id, @@ -82,29 +137,17 @@ syncer::UpdateResponseData CreateUpdateResponseData( base::Optional<std::string> guid = base::nullopt, const std::string& icon_url = std::string(), const std::string& icon_data = std::string()) { - if (!guid) - guid = base::GenerateGUID(); + UpdateResponseDataBuilder builder(server_id, parent_id, title, + unique_position); + if (guid) { + builder.SetGuid(*guid); + } + if (!is_folder) { + builder.SetUrl(GURL(url)); + } + builder.SetFavicon(GURL(icon_url), icon_data); - syncer::EntityData data; - data.id = server_id; - data.originator_client_item_id = *guid; - data.parent_id = parent_id; - data.unique_position = unique_position.ToProto(); - - sync_pb::BookmarkSpecifics* bookmark_specifics = - data.specifics.mutable_bookmark(); - bookmark_specifics->set_guid(*guid); - bookmark_specifics->set_legacy_canonicalized_title(title); - bookmark_specifics->set_url(url); - bookmark_specifics->set_icon_url(icon_url); - bookmark_specifics->set_favicon(icon_data); - - data.is_folder = is_folder; - syncer::UpdateResponseData response_data; - response_data.entity = std::move(data); - // Similar to what's done in the loopback_server. - response_data.response_version = 0; - return response_data; + return builder.Build(); } syncer::UpdateResponseData CreateBookmarkBarNodeUpdateData() { @@ -542,10 +585,12 @@ TEST(BookmarkModelMergerTest, syncer::UpdateResponseDataList updates; updates.push_back(CreateBookmarkBarNodeUpdateData()); - updates.push_back(CreateUpdateResponseData( - /*server_id=*/kId, /*parent_id=*/kBookmarkBarId, kRemoteTitle, - /*url=*/std::string(), - /*is_folder=*/true, /*unique_position=*/pos)); + updates.push_back(UpdateResponseDataBuilder(/*server_id=*/kId, + /*parent_id=*/kBookmarkBarId, + kRemoteTitle, + /*unique_position=*/pos) + .SetLegacyTitleOnly() + .Build()); std::unique_ptr<SyncedBookmarkTracker> tracker = Merge(std::move(updates), bookmark_model.get()); @@ -1952,4 +1997,53 @@ TEST(BookmarkModelMergerTest, ShouldRemoveDifferentFolderDuplicatesByGUID) { EXPECT_EQ(bookmark_bar_node->children().front()->children().size(), 2u); } +TEST(BookmarkModelMergerTest, ShouldReuploadBookmarkOnEmptyGuid) { + base::test::ScopedFeatureList override_features; + override_features.InitAndEnableFeature( + switches::kSyncReuploadBookmarkFullTitles); + + const std::string kFolder1Title = "folder1"; + const std::string kFolder2Title = "folder2"; + + const std::string kFolder1Id = "Folder1Id"; + const std::string kFolder2Id = "Folder2Id"; + + const std::string suffix = syncer::UniquePosition::RandomSuffix(); + const syncer::UniquePosition posFolder1 = + syncer::UniquePosition::InitialPosition(suffix); + const syncer::UniquePosition posFolder2 = + syncer::UniquePosition::After(posFolder1, suffix); + + std::unique_ptr<bookmarks::BookmarkModel> bookmark_model = + bookmarks::TestBookmarkClient::CreateModel(); + + // -------- The remote model -------- + syncer::UpdateResponseDataList updates; + updates.push_back(CreateBookmarkBarNodeUpdateData()); + updates.push_back(CreateUpdateResponseData( + /*server_id=*/kFolder1Id, /*parent_id=*/kBookmarkBarId, kFolder1Title, + /*url=*/std::string(), + /*is_folder=*/true, /*unique_position=*/posFolder1, + base::GenerateGUID())); + + // Mimic that the entity didn't have GUID in specifics. This entity should be + // reuploaded later. + updates.back().entity.is_bookmark_guid_in_specifics_preprocessed = true; + + updates.push_back(CreateUpdateResponseData( + /*server_id=*/kFolder2Id, /*parent_id=*/kBookmarkBarId, kFolder2Title, + /*url=*/std::string(), + /*is_folder=*/true, /*unique_position=*/posFolder2, + base::GenerateGUID())); + + std::unique_ptr<SyncedBookmarkTracker> tracker = + Merge(std::move(updates), bookmark_model.get()); + + ASSERT_THAT(tracker->GetEntityForSyncId(kFolder1Id), NotNull()); + ASSERT_THAT(tracker->GetEntityForSyncId(kFolder2Id), NotNull()); + + EXPECT_TRUE(tracker->GetEntityForSyncId(kFolder1Id)->IsUnsynced()); + EXPECT_FALSE(tracker->GetEntityForSyncId(kFolder2Id)->IsUnsynced()); +} + } // namespace sync_bookmarks diff --git a/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc b/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc index 6bf8d1b45d5..2cba3dd2cef 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc @@ -239,13 +239,37 @@ void BookmarkModelObserverImpl::BookmarkNodeFaviconChanged( const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode( node, model, /*force_favicon_load=*/false, entity->has_final_guid()); - if (entity->MatchesFaviconHash(specifics.bookmark().favicon())) { - // The favicon content didn't actually change, which means this event is - // almost certainly the result of favicon loading having completed. - return; + // Check that we do not ignore changes when there is actual favicon in + // specifics. + DCHECK(model->GetFaviconType(node) == favicon_base::IconType::kFavicon || + !specifics.bookmark().has_favicon()); + + // TODO(crbug.com/1094825): implement |base_specifics_hash| similar to + // ClientTagBasedModelTypeProcessor. + if (!entity->MatchesFaviconHash(specifics.bookmark().favicon())) { + // Skip any changes if the node has touch favicon (which is not used for the + // specifics). MatchesFaviconHash would return false in this case and the + // entity would been committed without any favicon. + if (model->GetFaviconType(node) == favicon_base::IconType::kFavicon || + model->GetFaviconType(node) == favicon_base::IconType::kInvalid || + !base::FeatureList::IsEnabled( + switches::kSyncIgnoreChangesInTouchIcons)) { + ProcessUpdate(entity, specifics); + return; + } } - ProcessUpdate(entity, specifics); + // The favicon content didn't actually change, which means this event is + // almost certainly the result of favicon loading having completed. + if (entity->IsUnsynced() && + base::FeatureList::IsEnabled( + switches::kSyncDoNotCommitBookmarksWithoutFavicon)) { + // When kSyncDoNotCommitBookmarksWithoutFavicon is enabled, nudge for + // commit once favicon is loaded. This is needed in case when unsynced + // entity was skipped while building commit requests (since favicon wasn't + // loaded). + nudge_for_commit_closure_.Run(); + } } void BookmarkModelObserverImpl::BookmarkNodeChildrenReordered( 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 fea8d20c838..3e41589b4b7 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc @@ -13,11 +13,14 @@ #include "base/strings/utf_string_conversions.h" #include "base/test/mock_callback.h" +#include "base/test/scoped_feature_list.h" #include "components/bookmarks/browser/bookmark_model.h" #include "components/bookmarks/test/test_bookmark_client.h" #include "components/sync/base/time.h" #include "components/sync/base/unique_position.h" #include "components/sync/driver/sync_driver_switches.h" +#include "components/sync_bookmarks/bookmark_specifics_conversions.h" +#include "components/sync_bookmarks/switches.h" #include "components/sync_bookmarks/synced_bookmark_tracker.h" #include "components/undo/bookmark_undo_service.h" #include "testing/gmock/include/gmock/gmock.h" @@ -85,6 +88,28 @@ class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient { return true; } + // Mimics the completion of a previously-triggered GetFaviconImageForPageURL() + // call for |page_url|, usually invoked by BookmarkModel. Returns false if no + // such a call is pending completion. The completion returns an empty image + // for the favicon. + bool SimulateEmptyFaviconLoaded(const GURL& page_url) { + if (requests_per_page_url_[page_url].empty()) { + return false; + } + + favicon_base::FaviconImageCallback callback = + std::move(requests_per_page_url_[page_url].front()); + requests_per_page_url_[page_url].pop_front(); + + std::move(callback).Run(favicon_base::FaviconImageResult()); + return true; + } + + // This may be used to mimic iOS device behaviour. + void SetPreferTouchIcon(bool prefer_touch_icon) { + prefer_touch_icon_ = prefer_touch_icon; + } + // bookmarks::TestBookmarkClient implementation. base::CancelableTaskTracker::TaskId GetFaviconImageForPageURL( const GURL& page_url, @@ -95,10 +120,14 @@ class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient { return next_task_id_++; } + // BookmarkClient overrides. + bool PreferTouchIcon() override { return prefer_touch_icon_; } + private: base::CancelableTaskTracker::TaskId next_task_id_ = 1; std::map<GURL, std::list<favicon_base::FaviconImageCallback>> requests_per_page_url_; + bool prefer_touch_icon_ = false; }; class BookmarkModelObserverImplTest : public testing::Test { @@ -777,6 +806,59 @@ TEST_F(BookmarkModelObserverImplTest, ShouldCommitLocalFaviconChange) { } TEST_F(BookmarkModelObserverImplTest, + ShouldNudgeForCommitOnFaviconLoadAfterRestart) { + base::test::ScopedFeatureList feature_list; + feature_list.InitAndEnableFeature( + switches::kSyncDoNotCommitBookmarksWithoutFavicon); + + const GURL kBookmarkUrl("http://www.url.com"); + const GURL kIconUrl("http://www.url.com/favicon.ico"); + const SkColor kColor = SK_ColorRED; + + // Simulate work after restart. Add a new bookmark to a model and its + // specifics to the tracker without loading favicon. + bookmark_model()->RemoveObserver(observer()); + + // Add a new node with specifics and mark it unsynced. + const bookmarks::BookmarkNode* bookmark_bar_node = + bookmark_model()->bookmark_bar_node(); + const bookmarks::BookmarkNode* bookmark_node = bookmark_model()->AddURL( + /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("title"), + kBookmarkUrl); + + sync_pb::EntitySpecifics specifics = + CreateSpecificsFromBookmarkNode(bookmark_node, bookmark_model(), + /*force_favicon_load=*/false, + /*include_guid=*/true); + const gfx::Image favicon_image = CreateTestImage(kColor); + scoped_refptr<base::RefCountedMemory> favicon_bytes = + favicon_image.As1xPNGBytes(); + specifics.mutable_bookmark()->set_favicon(favicon_bytes->front(), + favicon_bytes->size()); + specifics.mutable_bookmark()->set_icon_url(kIconUrl.spec()); + + const SyncedBookmarkTracker::Entity* entity = bookmark_tracker()->Add( + bookmark_node, "id", /*server_version=*/1, base::Time::Now(), + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()) + .ToProto(), + specifics); + bookmark_tracker()->IncrementSequenceNumber(entity); + + // Restore state. + bookmark_model()->AddObserver(observer()); + + // Currently there is the unsynced |entity| which has no loaded favicon. + ASSERT_FALSE(bookmark_node->is_favicon_loaded()); + ASSERT_TRUE(entity->IsUnsynced()); + + EXPECT_CALL(*nudge_for_commit_closure(), Run()); + bookmark_model()->GetFavicon(bookmark_node); + ASSERT_TRUE(bookmark_client()->SimulateFaviconLoaded(kBookmarkUrl, kIconUrl, + SK_ColorRED)); +} + +TEST_F(BookmarkModelObserverImplTest, ShouldAddRestoredBookmarkWhenTombstoneCommitMayHaveStarted) { const bookmarks::BookmarkNode* bookmark_bar_node = bookmark_model()->bookmark_bar_node(); @@ -821,6 +903,167 @@ TEST_F(BookmarkModelObserverImplTest, EXPECT_EQ(folder_entity->bookmark_node(), folder); } +// Tests that on iOS devices there is no reupload of bookmarks after remote +// update. This might happen in case when the remote bookmark contains favicon +// and the local model has touch icon (which is not used in specifics). +TEST_F(BookmarkModelObserverImplTest, + ShouldNotUpdateBookmarkOnTouchIconLoaded) { + bookmark_client()->SetPreferTouchIcon(true); + + const GURL kBookmarkUrl("http://www.url.com"); + const GURL kIconUrl("http://www.url.com/favicon.ico"); + const SkColor kColor = SK_ColorRED; + + // Simulate remote incremental update (without merge of favicon). + bookmark_model()->RemoveObserver(observer()); + + // Add a new node with specifics. + const bookmarks::BookmarkNode* bookmark_bar_node = + bookmark_model()->bookmark_bar_node(); + const bookmarks::BookmarkNode* bookmark_node = bookmark_model()->AddURL( + /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("title"), + kBookmarkUrl); + + sync_pb::EntitySpecifics specifics = + CreateSpecificsFromBookmarkNode(bookmark_node, bookmark_model(), + /*force_favicon_load=*/false, + /*include_guid=*/true); + + // Add favicon to the specifics to be sure that MatchesFaviconHash returns + // false. + const gfx::Image favicon_image = CreateTestImage(kColor); + scoped_refptr<base::RefCountedMemory> favicon_bytes = + favicon_image.As1xPNGBytes(); + specifics.mutable_bookmark()->set_favicon(favicon_bytes->front(), + favicon_bytes->size()); + specifics.mutable_bookmark()->set_icon_url(kIconUrl.spec()); + + const SyncedBookmarkTracker::Entity* entity = bookmark_tracker()->Add( + bookmark_node, "id", /*server_version=*/1, base::Time::Now(), + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()) + .ToProto(), + specifics); + + // Restore state. + bookmark_model()->AddObserver(observer()); + + ASSERT_FALSE(entity->IsUnsynced()); + + // Simulate BookmarkNodeFaviconChanged event (in normal case it would be + // called after storing remote favicon in the history backend). + // + // Invalidate bookmark favicon and load it again. + bookmark_model()->OnFaviconsChanged({kBookmarkUrl}, /*icon_url=*/GURL()); + ASSERT_TRUE(bookmark_node->is_favicon_loading()); + ASSERT_TRUE(bookmark_client()->SimulateFaviconLoaded(kBookmarkUrl, kIconUrl, + SK_ColorRED)); + ASSERT_EQ(bookmark_model()->GetFaviconType(bookmark_node), + favicon_base::IconType::kTouchIcon); + + EXPECT_FALSE(entity->IsUnsynced()); +} + +// Tests that there is still nudge for commit on touch icon loaded for the iOS +// platform. +TEST_F(BookmarkModelObserverImplTest, ShouldNudgeCommitOnTouchIconLoaded) { + base::test::ScopedFeatureList feature_list; + feature_list.InitAndEnableFeature( + switches::kSyncDoNotCommitBookmarksWithoutFavicon); + bookmark_client()->SetPreferTouchIcon(true); + + const GURL kBookmarkUrl("http://www.url.com"); + const GURL kIconUrl("http://www.url.com/favicon.ico"); + const SkColor kColor = SK_ColorRED; + + // Simulate remote incremental update (without merge of favicon). + bookmark_model()->RemoveObserver(observer()); + + // Add a new node with specifics and mark it unsynced. + const bookmarks::BookmarkNode* bookmark_bar_node = + bookmark_model()->bookmark_bar_node(); + const bookmarks::BookmarkNode* bookmark_node = bookmark_model()->AddURL( + /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("title"), + kBookmarkUrl); + + sync_pb::EntitySpecifics specifics = + CreateSpecificsFromBookmarkNode(bookmark_node, bookmark_model(), + /*force_favicon_load=*/false, + /*include_guid=*/true); + + // Add favicon to the specifics to be sure that MatchesFaviconHash returns + // false. + const gfx::Image favicon_image = CreateTestImage(kColor); + scoped_refptr<base::RefCountedMemory> favicon_bytes = + favicon_image.As1xPNGBytes(); + specifics.mutable_bookmark()->set_favicon(favicon_bytes->front(), + favicon_bytes->size()); + specifics.mutable_bookmark()->set_icon_url(kIconUrl.spec()); + + const SyncedBookmarkTracker::Entity* entity = bookmark_tracker()->Add( + bookmark_node, "id", /*server_version=*/1, base::Time::Now(), + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()) + .ToProto(), + specifics); + bookmark_tracker()->IncrementSequenceNumber(entity); + + // Restore state. + bookmark_model()->AddObserver(observer()); + + ASSERT_TRUE(entity->IsUnsynced()); + ASSERT_TRUE(entity->MatchesSpecificsHash(specifics)); + + EXPECT_CALL(*nudge_for_commit_closure(), Run()); + + // Simulate BookmarkNodeFaviconChanged event (in normal case it would be + // called after storing remote favicon in the history backend). + // + // Invalidate bookmark favicon and load it again. + bookmark_model()->OnFaviconsChanged({kBookmarkUrl}, /*icon_url=*/GURL()); + ASSERT_TRUE(bookmark_node->is_favicon_loading()); + ASSERT_TRUE(bookmark_client()->SimulateFaviconLoaded(kBookmarkUrl, kIconUrl, + SK_ColorRED)); + ASSERT_EQ(bookmark_model()->GetFaviconType(bookmark_node), + favicon_base::IconType::kTouchIcon); + + // Check that specifics haven't been changed. + EXPECT_TRUE(entity->MatchesSpecificsHash(specifics)); +} + +// Tests that the bookmark entity will be committed if its favicon is deleted. +TEST_F(BookmarkModelObserverImplTest, ShouldCommitOnDeleteFavicon) { + const GURL kBookmarkUrl("http://www.url.com"); + const GURL kIconUrl("http://www.url.com/favicon.ico"); + + // Add a new node with specifics. + const bookmarks::BookmarkNode* bookmark_bar_node = + bookmark_model()->bookmark_bar_node(); + const bookmarks::BookmarkNode* bookmark_node = bookmark_model()->AddURL( + /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("title"), + kBookmarkUrl); + + ASSERT_TRUE(bookmark_node->is_favicon_loading()); + ASSERT_TRUE(bookmark_client()->SimulateFaviconLoaded(kBookmarkUrl, kIconUrl, + SK_ColorRED)); + + const SyncedBookmarkTracker::Entity* entity = + bookmark_tracker()->GetEntityForBookmarkNode(bookmark_node); + ASSERT_THAT(entity, NotNull()); + ASSERT_TRUE(entity->IsUnsynced()); + + SimulateCommitResponseForAllLocalChanges(); + + ASSERT_FALSE(bookmark_tracker()->HasLocalChanges()); + + // Delete favicon and check that its deletion is committed. + bookmark_model()->OnFaviconsChanged({kBookmarkUrl}, GURL()); + ASSERT_TRUE(bookmark_node->is_favicon_loading()); + ASSERT_TRUE(bookmark_client()->SimulateEmptyFaviconLoaded(kBookmarkUrl)); + + EXPECT_TRUE(entity->IsUnsynced()); +} + } // namespace } // namespace sync_bookmarks diff --git a/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc b/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc index 8ae9986ac49..76df126fa63 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc @@ -9,6 +9,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/feature_list.h" +#include "base/metrics/histogram_functions.h" #include "base/strings/utf_string_conversions.h" #include "base/threading/sequenced_task_runner_handle.h" #include "base/trace_event/memory_usage_estimator.h" @@ -244,6 +245,9 @@ void BookmarkModelTypeProcessor::OnUpdateReceived( model_type_state.encryption_key_name(); bookmark_tracker_->set_model_type_state(model_type_state); updates_handler.Process(updates, got_new_encryption_requirements); + if (bookmark_tracker_->ReuploadBookmarksOnLoadIfNeeded()) { + NudgeForCommitIfNeeded(); + } // There are cases when we receive non-empty updates that don't result in // model changes (e.g. reflections). In that case, issue a write to persit the // progress marker in order to avoid downloading those updates again. @@ -252,6 +256,10 @@ void BookmarkModelTypeProcessor::OnUpdateReceived( // Schedule save just in case one is needed. schedule_save_closure_.Run(); } + + base::UmaHistogramCounts10000( + "Sync.BookmarksWithoutFullTitle.OnRemoteUpdate", + updates_handler.valid_updates_without_full_title_for_uma()); } const SyncedBookmarkTracker* BookmarkModelTypeProcessor::GetTrackerForTest() @@ -457,9 +465,13 @@ void BookmarkModelTypeProcessor::OnInitialUpdateReceived( bookmark_model_, bookmark_undo_service_, bookmark_model_observer_.get()); - BookmarkModelMerger(std::move(updates), bookmark_model_, favicon_service_, - bookmark_tracker_.get()) - .Merge(); + BookmarkModelMerger model_merger(std::move(updates), bookmark_model_, + favicon_service_, bookmark_tracker_.get()); + model_merger.Merge(); + + base::UmaHistogramCounts1M( + "Sync.BookmarksWithoutFullTitle.OnInitialMerge", + model_merger.valid_updates_without_full_title_for_uma()); } // If any of the permanent nodes is missing, we treat it as failure. 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 6885c71a364..f5311950fc1 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_type_processor_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_type_processor_unittest.cc @@ -19,6 +19,7 @@ #include "components/bookmarks/test/test_bookmark_client.h" #include "components/favicon/core/test/mock_favicon_service.h" #include "components/sync/base/unique_position.h" +#include "components/sync/engine/commit_queue.h" #include "components/sync/model/data_type_activation_request.h" #include "components/sync_bookmarks/switches.h" #include "components/undo/bookmark_undo_service.h" @@ -63,11 +64,13 @@ syncer::UpdateResponseData CreateUpdateResponseData( data.parent_id = bookmark_info.parent_id; data.server_defined_unique_tag = bookmark_info.server_tag; data.unique_position = unique_position.ToProto(); + data.originator_client_item_id = guid; sync_pb::BookmarkSpecifics* bookmark_specifics = data.specifics.mutable_bookmark(); bookmark_specifics->set_guid(guid); bookmark_specifics->set_legacy_canonicalized_title(bookmark_info.title); + bookmark_specifics->set_full_title(bookmark_info.title); if (bookmark_info.url.empty()) { data.is_folder = true; } else { @@ -148,6 +151,24 @@ void InitWithSyncedBookmarks(const std::vector<BookmarkInfo>& bookmarks, AssertState(processor, bookmarks); } +class MockCommitQueue : public syncer::CommitQueue { + public: + MOCK_METHOD0(NudgeForCommit, void()); +}; + +class ProxyCommitQueue : public syncer::CommitQueue { + public: + explicit ProxyCommitQueue(CommitQueue* commit_queue) + : commit_queue_(commit_queue) { + DCHECK(commit_queue_); + } + + void NudgeForCommit() override { commit_queue_->NudgeForCommit(); } + + private: + CommitQueue* commit_queue_ = nullptr; +}; + class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient { public: // This method must be used to tell the bookmark_model about favicon. @@ -191,17 +212,18 @@ class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient { class BookmarkModelTypeProcessorTest : public testing::Test { public: BookmarkModelTypeProcessorTest() - : processor_(&bookmark_undo_service_), + : processor_(std::make_unique<BookmarkModelTypeProcessor>( + &bookmark_undo_service_)), bookmark_model_(bookmarks::TestBookmarkClient::CreateModelWithClient( std::make_unique<TestBookmarkClientWithFavicon>())) { // TODO(crbug.com/516866): This class assumes model is loaded and sync has // started before running tests. We should test other variations (i.e. model // isn't loaded yet and/or sync didn't start yet). - processor_.SetFaviconService(&favicon_service_); + processor_->SetFaviconService(&favicon_service_); } void SimulateModelReadyToSync() { - processor_.ModelReadyToSync( + processor_->ModelReadyToSync( /*metadata_str=*/std::string(), schedule_save_closure_.Get(), bookmark_model_.get()); } @@ -209,7 +231,19 @@ class BookmarkModelTypeProcessorTest : public testing::Test { void SimulateOnSyncStarting() { syncer::DataTypeActivationRequest request; request.cache_guid = kCacheGuid; - processor_.OnSyncStarting(request, base::DoNothing()); + processor_->OnSyncStarting(request, base::DoNothing()); + } + + void SimulateConnectSync() { + processor_->ConnectSync( + std::make_unique<ProxyCommitQueue>(&mock_commit_queue_)); + } + + // Simulate browser restart. + void ResetModelTypeProcessor() { + processor_ = + std::make_unique<BookmarkModelTypeProcessor>(&bookmark_undo_service_); + processor_->SetFaviconService(&favicon_service_); } void DestroyBookmarkModel() { bookmark_model_.reset(); } @@ -223,7 +257,8 @@ class BookmarkModelTypeProcessorTest : public testing::Test { return &bookmark_undo_service_; } favicon::FaviconService* favicon_service() { return &favicon_service_; } - BookmarkModelTypeProcessor* processor() { return &processor_; } + MockCommitQueue* mock_commit_queue() { return &mock_commit_queue_; } + BookmarkModelTypeProcessor* processor() { return processor_.get(); } base::MockCallback<base::RepeatingClosure>* schedule_save_closure() { return &schedule_save_closure_; } @@ -241,7 +276,8 @@ class BookmarkModelTypeProcessorTest : public testing::Test { NiceMock<base::MockCallback<base::RepeatingClosure>> schedule_save_closure_; BookmarkUndoService bookmark_undo_service_; NiceMock<favicon::MockFaviconService> favicon_service_; - BookmarkModelTypeProcessor processor_; + NiceMock<MockCommitQueue> mock_commit_queue_; + std::unique_ptr<BookmarkModelTypeProcessor> processor_; std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_; }; @@ -539,6 +575,7 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldNotRecommitEntitiesWhenEncryptionIsUpToDate) { SimulateModelReadyToSync(); SimulateOnSyncStarting(); + SimulateConnectSync(); // Initialize the process to make sure the tracker has been created. InitWithSyncedBookmarks({}, processor()); const SyncedBookmarkTracker* tracker = processor()->GetTrackerForTest(); @@ -560,6 +597,7 @@ TEST_F(BookmarkModelTypeProcessorTest, /*response_version=*/0); response_data.encryption_key_name = kEncryptionKeyName; + EXPECT_CALL(*mock_commit_queue(), NudgeForCommit()).Times(0); syncer::UpdateResponseDataList updates; updates.push_back(std::move(response_data)); processor()->OnUpdateReceived(model_type_state, std::move(updates)); @@ -686,34 +724,49 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldReuploadLegacyBookmarksOnStart) { SimulateModelReadyToSync(); SimulateOnSyncStarting(); + SimulateConnectSync(); InitWithSyncedBookmarks(bookmarks, processor()); sync_pb::BookmarkModelMetadata model_metadata = BuildBookmarkModelMetadataWithoutFullTitles(); - // Ensure that bookmark is legacy. - ASSERT_FALSE(model_metadata.bookmarks_full_title_reuploaded()); - ASSERT_TRUE(processor() - ->GetTrackerForTest() - ->GetEntitiesWithLocalChanges(/*max_entries=*/1) - .empty()); + ASSERT_FALSE(processor()->GetTrackerForTest()->HasLocalChanges()); + + // Simulate browser restart, enable sync reupload and initialize the processor + // again. + ResetModelTypeProcessor(); base::test::ScopedFeatureList features; features.InitAndEnableFeature(switches::kSyncReuploadBookmarkFullTitles); - BookmarkModelTypeProcessor new_processor(bookmark_undo_service()); std::string metadata_str; model_metadata.SerializeToString(&metadata_str); - new_processor.ModelReadyToSync(metadata_str, base::DoNothing(), - bookmark_model()); + processor()->ModelReadyToSync(metadata_str, base::DoNothing(), + bookmark_model()); + SimulateOnSyncStarting(); + SimulateConnectSync(); + + ASSERT_THAT(processor()->GetTrackerForTest(), NotNull()); + const SyncedBookmarkTracker::Entity* entity = + processor()->GetTrackerForTest()->GetEntityForSyncId(kNodeId); + ASSERT_THAT(entity, NotNull()); + + // Entity should be synced before until first update is received. + ASSERT_FALSE(entity->IsUnsynced()); + ASSERT_FALSE(processor() + ->GetTrackerForTest() + ->BuildBookmarkModelMetadata() + .bookmarks_full_title_reuploaded()); + + // Synchronize with the server and get any updates. + EXPECT_CALL(*mock_commit_queue(), NudgeForCommit()); + processor()->OnUpdateReceived(CreateDummyModelTypeState(), + syncer::UpdateResponseDataList()); // Check that all entities are unsynced now and metadata is marked as // reuploaded. - const size_t entities_count = - processor()->GetTrackerForTest()->GetAllEntities().size(); - EXPECT_EQ(1u, new_processor.GetTrackerForTest() - ->GetEntitiesWithLocalChanges(entities_count) - .size()); - EXPECT_TRUE(new_processor.GetTrackerForTest() + EXPECT_TRUE(entity->IsUnsynced()); + EXPECT_TRUE(processor() + ->GetTrackerForTest() ->BuildBookmarkModelMetadata() .bookmarks_full_title_reuploaded()); } diff --git a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc index 5ccdcae3d0d..b3ec5b14c38 100644 --- a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc +++ b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc @@ -264,7 +264,12 @@ void BookmarkRemoteUpdatesHandler::Process( tracked_entity->metadata()->server_version() >= update->response_version && !local_guid_needs_update) { - // Seen this update before; just ignore it. + // Seen this update before. This update may be a reflection and may have + // missing the final GUID in specifics. Next reupload will populate GUID + // in specifics and this codepath will not repeat indefinitely. This logic + // is needed for the case when there is only one device and hence the GUID + // will not be set by other devices. + ReuploadEntityIfNeeded(update_entity, tracked_entity); continue; } @@ -549,7 +554,7 @@ BookmarkRemoteUpdatesHandler::ProcessCreate( bookmark_node, update_entity.id, update.response_version, update_entity.creation_time, update_entity.unique_position, update_entity.specifics); - ReuploadEntityIfNeeded(update_entity.specifics.bookmark(), entity); + ReuploadEntityIfNeeded(update_entity, entity); return entity; } @@ -606,7 +611,7 @@ void BookmarkRemoteUpdatesHandler::ProcessUpdate( } ApplyRemoteUpdate(update, tracked_entity, new_parent_entity, bookmark_model_, bookmark_tracker_, favicon_service_); - ReuploadEntityIfNeeded(update_entity.specifics.bookmark(), tracked_entity); + ReuploadEntityIfNeeded(update_entity, tracked_entity); } void BookmarkRemoteUpdatesHandler::ProcessDelete( @@ -730,7 +735,7 @@ void BookmarkRemoteUpdatesHandler::ProcessConflict( ApplyRemoteUpdate(update, tracked_entity, new_parent_entity, bookmark_model_, bookmark_tracker_, favicon_service_); } - ReuploadEntityIfNeeded(update_entity.specifics.bookmark(), tracked_entity); + ReuploadEntityIfNeeded(update_entity, tracked_entity); } void BookmarkRemoteUpdatesHandler::RemoveEntityAndChildrenFromTracker( @@ -755,12 +760,18 @@ const bookmarks::BookmarkNode* BookmarkRemoteUpdatesHandler::GetParentNode( } void BookmarkRemoteUpdatesHandler::ReuploadEntityIfNeeded( - const sync_pb::BookmarkSpecifics& specifics, + const syncer::EntityData& entity_data, const SyncedBookmarkTracker::Entity* tracked_entity) { - if (!IsFullTitleReuploadNeeded(specifics)) { - return; + DCHECK(tracked_entity); + DCHECK_EQ(tracked_entity->metadata()->server_id(), entity_data.id); + // Do not initiate reupload if the local entity is a tombstone or a permanent + // node. + if (tracked_entity->bookmark_node() && + !tracked_entity->bookmark_node()->is_permanent_node() && + IsBookmarkEntityReuploadNeeded(entity_data)) { + bookmark_tracker_->IncrementSequenceNumber(tracked_entity); + ++valid_updates_without_full_title_; } - bookmark_tracker_->IncrementSequenceNumber(tracked_entity); } } // namespace sync_bookmarks diff --git a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.h b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.h index 83070261c6c..6f71b9f12ea 100644 --- a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.h +++ b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.h @@ -52,6 +52,10 @@ class BookmarkRemoteUpdatesHandler { static std::vector<const syncer::UpdateResponseData*> ReorderUpdatesForTest( const syncer::UpdateResponseDataList* updates); + size_t valid_updates_without_full_title_for_uma() const { + return valid_updates_without_full_title_; + } + private: // Reorders incoming updates such that parent creation is before child // creation and child deletion is before parent deletion, and deletions should @@ -106,14 +110,19 @@ class BookmarkRemoteUpdatesHandler { // from |bookmark_tracker_|. void RemoveEntityAndChildrenFromTracker(const bookmarks::BookmarkNode* node); + // Initiate reupload for the update with |entity_data|. |tracked_entity| must + // not be nullptr. void ReuploadEntityIfNeeded( - const sync_pb::BookmarkSpecifics& specifics, + const syncer::EntityData& entity_data, const SyncedBookmarkTracker::Entity* tracked_entity); bookmarks::BookmarkModel* const bookmark_model_; favicon::FaviconService* const favicon_service_; SyncedBookmarkTracker* const bookmark_tracker_; + // Counts number of initiated reuploads. + size_t valid_updates_without_full_title_ = 0; + DISALLOW_COPY_AND_ASSIGN(BookmarkRemoteUpdatesHandler); }; 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 57fe90b5927..a927aec1489 100644 --- a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc @@ -23,6 +23,7 @@ #include "components/sync/model/conflict_resolution.h" #include "components/sync/protocol/unique_position.pb.h" #include "components/sync_bookmarks/bookmark_model_merger.h" +#include "components/sync_bookmarks/bookmark_specifics_conversions.h" #include "components/sync_bookmarks/switches.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -99,6 +100,7 @@ sync_pb::BookmarkModelMetadata CreateMetadataForPermanentNodes( const bookmarks::BookmarkModel* bookmark_model) { sync_pb::BookmarkModelMetadata model_metadata; model_metadata.mutable_model_type_state()->set_initial_sync_done(true); + model_metadata.set_bookmarks_full_title_reuploaded(true); *model_metadata.add_bookmarks_metadata() = CreateNodeMetadata(bookmark_model->bookmark_bar_node()->id(), @@ -109,6 +111,7 @@ sync_pb::BookmarkModelMetadata CreateMetadataForPermanentNodes( *model_metadata.add_bookmarks_metadata() = CreateNodeMetadata(bookmark_model->other_node()->id(), /*server_id=*/kOtherBookmarksId); + return model_metadata; } @@ -132,6 +135,7 @@ syncer::UpdateResponseData CreateUpdateResponseData( data.specifics.mutable_bookmark(); bookmark_specifics->set_guid(guid); bookmark_specifics->set_legacy_canonicalized_title(title); + bookmark_specifics->set_full_title(title); } data.is_folder = true; syncer::UpdateResponseData response_data; @@ -1732,8 +1736,10 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest, std::unique_ptr<SyncedBookmarkTracker> tracker = SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( bookmark_model(), std::move(model_metadata)); + ASSERT_THAT(tracker, NotNull()); ASSERT_EQ(5u, tracker->GetAllEntities().size()); + ASSERT_FALSE(tracker->GetEntityForSyncId(kLocalId)->IsUnsynced()); // Generate an update with two different ids. syncer::UpdateResponseDataList updates; @@ -1888,7 +1894,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest, /*server_id=*/kServerId, /*parent_id=*/kBookmarkBarId, /*guid=*/kLocalId, - /*title=*/"", + /*title=*/"Title", /*is_deletion=*/true, /*version=*/1, /*unique_position=*/ @@ -1907,6 +1913,128 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest, EXPECT_TRUE(bookmark_bar_node->children().empty()); } +TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest, + ShouldReuploadBookmarkOnEmptyGuid) { + base::test::ScopedFeatureList override_features; + override_features.InitAndEnableFeature( + switches::kSyncReuploadBookmarkFullTitles); + + const std::string kFolder1Title = "folder1"; + const std::string kFolder2Title = "folder2"; + + const std::string kFolder1Id = "Folder1Id"; + const std::string kFolder2Id = "Folder2Id"; + + syncer::UpdateResponseDataList updates; + updates.push_back(CreateUpdateResponseData( + /*server_id=*/kFolder1Id, + /*parent_id=*/kBookmarkBarId, + /*guid=*/base::GenerateGUID(), + /*title=*/kFolder1Title, + /*is_deletion=*/false, + /*version=*/0, + /*unique_position=*/ + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()))); + + // Remove GUID field for the first item only. + updates.back().entity.is_bookmark_guid_in_specifics_preprocessed = true; + + updates.push_back(CreateUpdateResponseData( + /*server_id=*/kFolder2Id, + /*parent_id=*/kBookmarkBarId, + /*guid=*/base::GenerateGUID(), + /*title=*/kFolder2Title, + /*is_deletion=*/false, + /*version=*/0, + /*unique_position=*/ + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()))); + ASSERT_FALSE( + updates.back().entity.is_bookmark_guid_in_specifics_preprocessed); + + updates_handler()->Process(std::move(updates), + /*got_new_encryption_requirements=*/false); + + const SyncedBookmarkTracker::Entity* entity1 = + tracker()->GetEntityForSyncId(kFolder1Id); + const SyncedBookmarkTracker::Entity* entity2 = + tracker()->GetEntityForSyncId(kFolder2Id); + ASSERT_THAT(entity1, NotNull()); + ASSERT_THAT(entity2, NotNull()); + ASSERT_TRUE(entity1->has_final_guid()); + + EXPECT_TRUE(entity1->IsUnsynced()); + EXPECT_FALSE(entity2->IsUnsynced()); +} + +// Tests that the reflection which doesn't have GUID in specifics will be +// reuploaded. +TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest, + ShouldReuploadBookmarkOnEmptyGuidForReflection) { + base::test::ScopedFeatureList override_features; + override_features.InitAndEnableFeature( + switches::kSyncReuploadBookmarkFullTitles); + + const std::string kFolderTitle = "folder"; + const std::string kFolderId = "FolderId"; + const std::string kGuid = base::GenerateGUID(); + + // Prepare local bookmarks data with entity which doesn't have final GUID. + const bookmarks::BookmarkNode* bookmark_bar_node = + bookmark_model()->bookmark_bar_node(); + const bookmarks::BookmarkNode* node = bookmark_model()->AddFolder( + bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kFolderTitle), + /*meta_info=*/nullptr, kGuid); + sync_pb::BookmarkModelMetadata model_metadata = + CreateMetadataForPermanentNodes(bookmark_model()); + sync_pb::BookmarkMetadata* node_metadata = + model_metadata.add_bookmarks_metadata(); + *node_metadata = + CreateNodeMetadata(node->id(), kFolderId, + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix())); + ASSERT_FALSE(node_metadata->metadata().has_client_tag_hash()); + const int64_t server_version = node_metadata->metadata().server_version(); + + std::unique_ptr<SyncedBookmarkTracker> tracker = + SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( + bookmark_model(), std::move(model_metadata)); + ASSERT_THAT(tracker, NotNull()); + ASSERT_EQ(4u, tracker->GetAllEntities().size()); + + const SyncedBookmarkTracker::Entity* entity = + tracker->GetEntityForSyncId(kFolderId); + ASSERT_THAT(entity, NotNull()); + ASSERT_FALSE(entity->IsUnsynced()); + ASSERT_FALSE(entity->has_final_guid()); + + syncer::UpdateResponseDataList updates; + // Create an update with the same server version as local entity has. This + // will simulate processing of reflection. + updates.push_back(CreateUpdateResponseData( + /*server_id=*/kFolderId, + /*parent_id=*/kBookmarkBarId, + /*guid=*/kGuid, + /*title=*/kFolderTitle, + /*is_deletion=*/false, + /*version=*/server_version, + /*unique_position=*/ + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()))); + + updates.back().entity.is_bookmark_guid_in_specifics_preprocessed = true; + + BookmarkRemoteUpdatesHandler updates_handler( + bookmark_model(), favicon_service(), tracker.get()); + updates_handler.Process(std::move(updates), + /*got_new_encryption_requirements=*/false); + ASSERT_EQ(entity, tracker->GetEntityForSyncId(kFolderId)); + + EXPECT_TRUE(entity->IsUnsynced()); + EXPECT_TRUE(entity->has_final_guid()); +} + } // namespace } // namespace sync_bookmarks diff --git a/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc b/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc index 1f4de797deb..4318c6175a7 100644 --- a/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc +++ b/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc @@ -12,6 +12,7 @@ #include "base/containers/span.h" #include "base/guid.h" #include "base/hash/sha1.h" +#include "base/logging.h" #include "base/metrics/histogram_functions.h" #include "base/strings/strcat.h" #include "base/strings/string_util.h" @@ -21,6 +22,7 @@ #include "components/bookmarks/browser/bookmark_node.h" #include "components/favicon/core/favicon_service.h" #include "components/sync/engine/engine_util.h" +#include "components/sync/model/entity_data.h" #include "components/sync/protocol/sync.pb.h" #include "components/sync_bookmarks/switches.h" #include "ui/gfx/favicon_size.h" @@ -185,8 +187,16 @@ std::string FullTitleToLegacyCanonicalizedTitle(const std::string& node_title) { return specifics_title; } -bool IsFullTitleReuploadNeeded(const sync_pb::BookmarkSpecifics& specifics) { - if (specifics.has_full_title()) { +bool IsBookmarkEntityReuploadNeeded( + const syncer::EntityData& remote_entity_data) { + DCHECK(remote_entity_data.server_defined_unique_tag.empty()); + // Do not initiate a reupload for a remote deletion. + if (remote_entity_data.is_deleted()) { + return false; + } + DCHECK(remote_entity_data.specifics.has_bookmark()); + if (remote_entity_data.specifics.bookmark().has_full_title() && + !remote_entity_data.is_bookmark_guid_in_specifics_preprocessed) { return false; } return base::FeatureList::IsEnabled( diff --git a/chromium/components/sync_bookmarks/bookmark_specifics_conversions.h b/chromium/components/sync_bookmarks/bookmark_specifics_conversions.h index 6db38b51039..8332fbda690 100644 --- a/chromium/components/sync_bookmarks/bookmark_specifics_conversions.h +++ b/chromium/components/sync_bookmarks/bookmark_specifics_conversions.h @@ -18,6 +18,10 @@ class BookmarkSpecifics; class EntitySpecifics; } // namespace sync_pb +namespace syncer { +struct EntityData; +} // namespace syncer + namespace favicon { class FaviconService; } // namespace favicon @@ -28,9 +32,9 @@ namespace sync_bookmarks { // truncating and the appending ' ' in some cases. std::string FullTitleToLegacyCanonicalizedTitle(const std::string& node_title); -// Used to decide if entity needs to be reuploaded for each remote change which -// is true if the proto field for the full title is missing. -bool IsFullTitleReuploadNeeded(const sync_pb::BookmarkSpecifics& specifics); +// Used to decide if entity needs to be reuploaded for each remote change. +bool IsBookmarkEntityReuploadNeeded( + const syncer::EntityData& remote_entity_data); // TODO(crbug.com/978430): Remove argument |include_guid| once the client tag // hash is required to be populated during sync metadata validation upon diff --git a/chromium/components/sync_bookmarks/switches.cc b/chromium/components/sync_bookmarks/switches.cc index 13fe6995911..1617ec0c49a 100644 --- a/chromium/components/sync_bookmarks/switches.cc +++ b/chromium/components/sync_bookmarks/switches.cc @@ -7,8 +7,7 @@ namespace switches { const base::Feature kSyncDoNotCommitBookmarksWithoutFavicon = { - "SyncDoNotCommitBookmarksWithoutFavicon", - base::FEATURE_DISABLED_BY_DEFAULT}; + "SyncDoNotCommitBookmarksWithoutFavicon", base::FEATURE_ENABLED_BY_DEFAULT}; // Enables updating a BookmarkNode's GUID by replacing the node itself. const base::Feature kUpdateBookmarkGUIDWithNodeReplacement{ @@ -29,4 +28,7 @@ const base::Feature kSyncDeduplicateAllBookmarksWithSameGUID{ "SyncDeduplicateAllBookmarksWithSameGUID", base::FEATURE_ENABLED_BY_DEFAULT}; +const base::Feature kSyncIgnoreChangesInTouchIcons{ + "SyncIgnoreChangesInTouchIcons", base::FEATURE_ENABLED_BY_DEFAULT}; + } // namespace switches diff --git a/chromium/components/sync_bookmarks/switches.h b/chromium/components/sync_bookmarks/switches.h index 7a9faface44..5828981f3e8 100644 --- a/chromium/components/sync_bookmarks/switches.h +++ b/chromium/components/sync_bookmarks/switches.h @@ -19,6 +19,8 @@ extern const base::Feature kSyncReuploadBookmarkFullTitles; extern const base::Feature kSyncProcessBookmarkRestoreAfterDeletion; // This switch is used to disable removing of bookmark duplicates by GUID. extern const base::Feature kSyncDeduplicateAllBookmarksWithSameGUID; +// TODO(crbug.com/1075709): remove after launch. +extern const base::Feature kSyncIgnoreChangesInTouchIcons; } // namespace switches diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc b/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc index 0f9f82a483d..e8e8a5bb332 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc @@ -30,7 +30,7 @@ namespace sync_bookmarks { const base::Feature kInvalidateBookmarkSyncMetadataIfMismatchingGuid{ "InvalidateBookmarkSyncMetadataIfMismatchingGuid", - base::FEATURE_DISABLED_BY_DEFAULT}; + base::FEATURE_ENABLED_BY_DEFAULT}; extern const base::Feature kInvalidateBookmarkSyncMetadataIfClientTagDuplicates{ "InvalidateBookmarkSyncMetadataIfClientTagDuplicates", @@ -190,12 +190,17 @@ SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( return nullptr; } - const bool bookmarks_full_title_reuploaded = - model_metadata.bookmarks_full_title_reuploaded(); + auto tracker = + CreateEmpty(std::move(*model_metadata.mutable_model_type_state())); - auto tracker = base::WrapUnique(new SyncedBookmarkTracker( - std::move(*model_metadata.mutable_model_type_state()), - bookmarks_full_title_reuploaded)); + // When the reupload feature is enabled and disabled again, there may occur + // new entities which weren't reuploaded. + const bool bookmarks_full_title_reuploaded = + model_metadata.bookmarks_full_title_reuploaded() && + base::FeatureList::IsEnabled(switches::kSyncReuploadBookmarkFullTitles); + if (bookmarks_full_title_reuploaded) { + tracker->SetBookmarksFullTitleReuploaded(); + } const CorruptionReason corruption_reason = tracker->InitEntitiesFromModelAndMetadata(model, @@ -208,13 +213,15 @@ SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( return nullptr; } - tracker->ReuploadBookmarksOnLoadIfNeeded(); - return tracker; } SyncedBookmarkTracker::~SyncedBookmarkTracker() = default; +void SyncedBookmarkTracker::SetBookmarksFullTitleReuploaded() { + bookmarks_full_title_reuploaded_ = true; +} + const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId( const std::string& sync_id) const { auto it = sync_id_to_entities_map_.find(sync_id); @@ -663,11 +670,11 @@ SyncedBookmarkTracker::ReorderUnsyncedEntitiesExceptDeletions( return ordered_entities; } -void SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() { +bool SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() { if (bookmarks_full_title_reuploaded_ || !base::FeatureList::IsEnabled( switches::kSyncReuploadBookmarkFullTitles)) { - return; + return false; } for (const auto& sync_id_and_entity : sync_id_to_entities_map_) { const SyncedBookmarkTracker::Entity* entity = @@ -680,7 +687,8 @@ void SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() { } IncrementSequenceNumber(entity); } - bookmarks_full_title_reuploaded_ = true; + SetBookmarksFullTitleReuploaded(); + return true; } void SyncedBookmarkTracker::TraverseAndAppend( diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker.h b/chromium/components/sync_bookmarks/synced_bookmark_tracker.h index 49ddc5406f2..6bf8d49a2cc 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker.h +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker.h @@ -148,6 +148,10 @@ class SyncedBookmarkTracker { ~SyncedBookmarkTracker(); + // This method is used to denote that all bookmarks are reuploaded and there + // is no need to reupload them again after next browser startup. + void SetBookmarksFullTitleReuploaded(); + // Returns null if no entity is found. const Entity* GetEntityForSyncId(const std::string& sync_id) const; @@ -279,6 +283,15 @@ class SyncedBookmarkTracker { void CheckAllNodesTracked( const bookmarks::BookmarkModel* bookmark_model) const; + // This method is used to mark all entities except permanent nodes as + // unsynced. This will cause reuploading of all bookmarks. The reupload + // will be initiated only when the |bookmarks_full_title_reuploaded| field in + // BookmarksMetadata is false. This field is used to prevent reuploading after + // each browser restart. Returns true if the reupload was initiated. + // TODO(crbug.com/1066962): remove this code when most of bookmarks are + // reuploaded. + bool ReuploadBookmarksOnLoadIfNeeded(); + private: // Enumeration of possible reasons why persisted metadata are considered // corrupted and don't match the bookmark model. Used in UMA metrics. Do not @@ -322,16 +335,6 @@ class SyncedBookmarkTracker { std::vector<const Entity*> ReorderUnsyncedEntitiesExceptDeletions( const std::vector<const Entity*>& entities) const; - // This method is used to mark all entities except permanent nodes as - // unsynced. This will cause reuploading of all bookmarks. This reupload - // should be initiated only when the |bookmarks_full_title_reuploaded| field - // in BookmarksMetadata is false. This field is used to prevent reuploading - // after each browser restart. It is set to true in - // BuildBookmarkModelMetadata. - // TODO(crbug.com/1066962): remove this code when most of bookmarks are - // reuploaded. - void ReuploadBookmarksOnLoadIfNeeded(); - // Recursive method that starting from |node| appends all corresponding // entities with updates in top-down order to |ordered_entities|. void TraverseAndAppend(const bookmarks::BookmarkNode* node, diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc b/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc index d73a2add0e1..a6be09a3b9b 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc @@ -16,6 +16,7 @@ #include "components/sync/base/time.h" #include "components/sync/base/unique_position.h" #include "components/sync/model/entity_data.h" +#include "components/sync_bookmarks/switches.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -442,6 +443,39 @@ TEST(SyncedBookmarkTrackerTest, Eq(kId1)); } +TEST(SyncedBookmarkTrackerTest, ShouldUndeleteTombstone) { + std::unique_ptr<SyncedBookmarkTracker> tracker = + SyncedBookmarkTracker::CreateEmpty(sync_pb::ModelTypeState()); + + const std::string kSyncId = "SYNC_ID"; + const int64_t kId = 1; + const int64_t kServerVersion = 1000; + const base::Time kModificationTime(base::Time::Now() - + base::TimeDelta::FromSeconds(1)); + const sync_pb::UniquePosition unique_position; + const sync_pb::EntitySpecifics specifics = + GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string()); + bookmarks::BookmarkNode node(kId, base::GenerateGUID(), GURL()); + const SyncedBookmarkTracker::Entity* entity = + tracker->Add(&node, kSyncId, kServerVersion, kModificationTime, + unique_position, specifics); + + ASSERT_THAT(tracker->TrackedUncommittedTombstonesCountForDebugging(), Eq(0U)); + + // Delete the bookmark, leading to a pending deletion (local tombstone). + tracker->MarkDeleted(tracker->GetEntityForSyncId(kSyncId)); + ASSERT_THAT(entity->bookmark_node(), IsNull()); + ASSERT_TRUE(entity->metadata()->is_deleted()); + ASSERT_THAT(tracker->TrackedUncommittedTombstonesCountForDebugging(), Eq(1U)); + + // Undelete it. + tracker->UndeleteTombstoneForBookmarkNode(entity, &node); + + EXPECT_THAT(entity->bookmark_node(), NotNull()); + EXPECT_FALSE(entity->metadata()->is_deleted()); + EXPECT_THAT(tracker->TrackedUncommittedTombstonesCountForDebugging(), Eq(0U)); +} + TEST(SyncedBookmarkTrackerTest, ShouldOrderParentUpdatesBeforeChildUpdatesAndDeletionsComeLast) { const size_t kMaxEntries = 1000; @@ -960,6 +994,94 @@ TEST(SyncedBookmarkTrackerTest, ShouldPopulateFaviconHashExplicitly) { EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes)); } +TEST(SyncedBookmarkTrackerTest, ShouldNotReuploadEntitiesAfterMergeAndRestart) { + base::test::ScopedFeatureList override_features; + override_features.InitAndEnableFeature( + switches::kSyncReuploadBookmarkFullTitles); + const std::string kTitle = "Title"; + const GURL kUrl("http://www.foo.com"); + + sync_pb::ModelTypeState model_type_state; + model_type_state.set_initial_sync_done(true); + std::unique_ptr<SyncedBookmarkTracker> tracker = + SyncedBookmarkTracker::CreateEmpty(model_type_state); + tracker->SetBookmarksFullTitleReuploaded(); + + 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); + + const sync_pb::EntitySpecifics specifics = + GenerateSpecifics(kTitle, kUrl.spec()); + tracker->Add(node, /*sync_id=*/"id", /*server_version=*/0, + /*creation_time=*/base::Time::Now(), + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()) + .ToProto(), + specifics); + + sync_pb::EntitySpecifics permanent_specifics; + permanent_specifics.mutable_bookmark(); + + // Add permanent nodes to tracker. + tracker->Add(model->bookmark_bar_node(), kBookmarkBarId, /*server_version=*/0, + /*creation_time=*/base::Time::Now(), + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()) + .ToProto(), + permanent_specifics); + tracker->Add(model->other_node(), kOtherBookmarksId, /*server_version=*/0, + /*creation_time=*/base::Time::Now(), + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()) + .ToProto(), + permanent_specifics); + tracker->Add(model->mobile_node(), kMobileBookmarksId, /*server_version=*/0, + /*creation_time=*/base::Time::Now(), + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()) + .ToProto(), + permanent_specifics); + + ASSERT_FALSE(tracker->HasLocalChanges()); + + // Simulate browser restart. + tracker = SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( + model.get(), tracker->BuildBookmarkModelMetadata()); + ASSERT_THAT(tracker, NotNull()); + EXPECT_FALSE(tracker->HasLocalChanges()); + EXPECT_EQ(4u, tracker->TrackedEntitiesCountForTest()); +} + +TEST(SyncedBookmarkTrackerTest, + ShouldResetReuploadFlagOnDisabledFeatureToggle) { + base::test::ScopedFeatureList override_features; + override_features.InitAndDisableFeature( + switches::kSyncReuploadBookmarkFullTitles); + + const std::string kTitle = "Title"; + const GURL kUrl("http://www.foo.com"); + + std::unique_ptr<bookmarks::BookmarkModel> bookmark_model = + bookmarks::TestBookmarkClient::CreateModel(); + + sync_pb::ModelTypeState model_type_state; + model_type_state.set_initial_sync_done(true); + sync_pb::BookmarkModelMetadata initial_model_metadata = + CreateMetadataForPermanentNodes(bookmark_model.get()); + initial_model_metadata.set_bookmarks_full_title_reuploaded(true); + std::unique_ptr<SyncedBookmarkTracker> tracker = + SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( + bookmark_model.get(), std::move(initial_model_metadata)); + ASSERT_THAT(tracker, NotNull()); + + EXPECT_FALSE( + tracker->BuildBookmarkModelMetadata().bookmarks_full_title_reuploaded()); +} + } // namespace } // namespace sync_bookmarks |