From 654bf550ee52be55e7bb0bce7b4c11244e763be7 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 21 Dec 2018 12:08:17 +0200 Subject: [PATCH 1/2] Clear order map after all portions sent --- browser/extensions/api/brave_sync_event_router.cc | 9 +++++++++ browser/extensions/api/brave_sync_event_router.h | 2 ++ common/extensions/api/brave_sync.json | 5 +++++ .../brave_sync/client/bookmark_change_processor.cc | 1 + components/brave_sync/client/brave_sync_client.h | 2 ++ components/brave_sync/client/brave_sync_client_impl.cc | 5 +++++ components/brave_sync/client/brave_sync_client_impl.h | 2 ++ components/brave_sync/extension/background.js | 6 +++++- 8 files changed, 31 insertions(+), 1 deletion(-) diff --git a/browser/extensions/api/brave_sync_event_router.cc b/browser/extensions/api/brave_sync_event_router.cc index 528c0de06592..f4ce1d3f3792 100644 --- a/browser/extensions/api/brave_sync_event_router.cc +++ b/browser/extensions/api/brave_sync_event_router.cc @@ -142,4 +142,13 @@ void BraveSyncEventRouter::LoadClient() { event_router_->BroadcastEvent(std::move(event)); } +void BraveSyncEventRouter::ClearOrderMap() { + auto args = std::make_unique(); + std::unique_ptr event( + new Event(extensions::events::FOR_TEST, + extensions::api::brave_sync::OnClearOrderMap::kEventName, + std::move(args))); + event_router_->BroadcastEvent(std::move(event)); +} + } // namespace extensions diff --git a/browser/extensions/api/brave_sync_event_router.h b/browser/extensions/api/brave_sync_event_router.h index 3e106073b01c..eac52983442b 100644 --- a/browser/extensions/api/brave_sync_event_router.h +++ b/browser/extensions/api/brave_sync_event_router.h @@ -57,6 +57,8 @@ class BraveSyncEventRouter { void NeedSyncWords(const std::string& seed); + void ClearOrderMap(); + void LoadClient(); private: diff --git a/common/extensions/api/brave_sync.json b/common/extensions/api/brave_sync.json index 20d31b76f962..2eca03683b91 100644 --- a/common/extensions/api/brave_sync.json +++ b/common/extensions/api/brave_sync.json @@ -288,6 +288,11 @@ "type": "function", "description": "Browser informs extension page to load js sync library.", "parameters": [] + }, + { + "name": "onClearOrderMap", + "type": "function", + "description": "Browser informs extension page all portions of bookmarks are sent, so it is time to clear the order map" } ], "functions": [ diff --git a/components/brave_sync/client/bookmark_change_processor.cc b/components/brave_sync/client/bookmark_change_processor.cc index 8fb1bd63e225..db1c70bacbf0 100644 --- a/components/brave_sync/client/bookmark_change_processor.cc +++ b/components/brave_sync/client/bookmark_change_processor.cc @@ -827,6 +827,7 @@ void BookmarkChangeProcessor::SendUnsynced( jslib_const::SyncRecordType_BOOKMARKS, records); records.clear(); } + sync_client_->ClearOrderMap(); } void BookmarkChangeProcessor::InitialSync() {} diff --git a/components/brave_sync/client/brave_sync_client.h b/components/brave_sync/client/brave_sync_client.h index d6ccea91cec2..3095659a5cd6 100644 --- a/components/brave_sync/client/brave_sync_client.h +++ b/components/brave_sync/client/brave_sync_client.h @@ -85,6 +85,8 @@ class BraveSyncClient { virtual void OnExtensionInitialized() = 0; virtual void OnSyncEnabledChanged() = 0; + + virtual void ClearOrderMap() = 0; }; } // namespace brave_sync diff --git a/components/brave_sync/client/brave_sync_client_impl.cc b/components/brave_sync/client/brave_sync_client_impl.cc index 81baff61afbd..b0cb3ce7fd02 100644 --- a/components/brave_sync/client/brave_sync_client_impl.cc +++ b/components/brave_sync/client/brave_sync_client_impl.cc @@ -191,4 +191,9 @@ void BraveSyncClientImpl::OnExtensionSystemReady() { } }; +void BraveSyncClientImpl::ClearOrderMap() { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + brave_sync_event_router_->ClearOrderMap(); +} + } // namespace brave_sync diff --git a/components/brave_sync/client/brave_sync_client_impl.h b/components/brave_sync/client/brave_sync_client_impl.h index 4439b51d529d..45dc30c2c894 100644 --- a/components/brave_sync/client/brave_sync_client_impl.h +++ b/components/brave_sync/client/brave_sync_client_impl.h @@ -78,6 +78,8 @@ class BraveSyncClientImpl : public BraveSyncClient, void LoadOrUnloadExtension(bool load); void OnExtensionSystemReady(); + void ClearOrderMap() override; + SyncMessageHandler* handler_; // not owned Profile* profile_; // not owned std::unique_ptr sync_prefs_; diff --git a/components/brave_sync/extension/background.js b/components/brave_sync/extension/background.js index 9f75aceb0041..84e039e86bcf 100644 --- a/components/brave_sync/extension/background.js +++ b/components/brave_sync/extension/background.js @@ -60,7 +60,6 @@ chrome.braveSync.onSendSyncRecords.addListener(function(category_name, records) if (category_name == 'BOOKMARKS') { fixupSyncRecordsArrayExtensionToBrowser(records); chrome.braveSync.resolvedSyncRecords(category_name, records); - orderMap = {}; } }); @@ -82,6 +81,10 @@ chrome.braveSync.onLoadClient.addListener(function() { LoadJsLibScript(); }); +chrome.braveSync.onClearOrderMap.addListener(function() { + orderMap = {}; +}); + chrome.braveSync.extensionInitialized(); console.log("chrome.braveSync.extensionInitialized"); @@ -96,6 +99,7 @@ function getOrder(record) { orderMap[record.objectId] = order; getBookmarkOrderCallback = null; } + var prevOrder = record.bookmark.prevOrder; var parentOrder = record.bookmark.parentOrder; if (!prevOrder && orderMap[record.bookmark.prevObjectId]) From 69180c1a9790d5e97953fd7da21bcfe3604e05a8 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 21 Dec 2018 14:01:36 +0200 Subject: [PATCH 2/2] Updated unit tests for clearing order map method --- .../client/bookmark_change_processor_unittest.cc | 10 ++++++++++ components/brave_sync/test_util.h | 1 + 2 files changed, 11 insertions(+) diff --git a/components/brave_sync/client/bookmark_change_processor_unittest.cc b/components/brave_sync/client/bookmark_change_processor_unittest.cc index 716d1429ff96..15f79504ed30 100644 --- a/components/brave_sync/client/bookmark_change_processor_unittest.cc +++ b/components/brave_sync/client/bookmark_change_processor_unittest.cc @@ -253,6 +253,7 @@ TEST_F(BraveBookmarkChangeProcessorTest, ResetClearMeta) { AddSimpleHierarchy(&folder1, &node_a, &node_b, &node_c); EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", _)).Times(1); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); EXPECT_TRUE(HasAnySyncMetaInfo(folder1)); @@ -291,6 +292,7 @@ TEST_F(BraveBookmarkChangeProcessorTest, ResetPreserveMeta) { AddSimpleHierarchy(&folder1, &node_a, &node_b, &node_c); EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", _)).Times(1); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); EXPECT_TRUE(HasAnySyncMetaInfo(folder1)); @@ -333,6 +335,7 @@ void BraveBookmarkChangeProcessorTest::BookmarkAddedImpl() { using brave_sync::jslib::SyncRecord; EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", ContainsRecord(SyncRecord::Action::A_CREATE, "https://a.com/"))).Times(1); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); } @@ -353,6 +356,7 @@ TEST_F(BraveBookmarkChangeProcessorTest, BookmarkDeleted) { EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", ContainsRecord(SyncRecord::Action::A_DELETE, "https://a.com/"))).Times(1); model()->Remove(nodes.at(0)); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); EXPECT_FALSE(GetDeletedNodeRoot()->IsVisible()); } @@ -370,6 +374,7 @@ TEST_F(BraveBookmarkChangeProcessorTest, BookmarkModified) { EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", ContainsRecord(SyncRecord::Action::A_UPDATE, "https://a-m.com/"))).Times(1); model()->SetURL(nodes.at(0), GURL("https://a-m.com/")); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); } @@ -390,6 +395,7 @@ TEST_F(BraveBookmarkChangeProcessorTest, BookmarkMovedInFolder) { EXPECT_EQ(intex_c, 2); EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", _)).Times(1); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); EXPECT_TRUE(HasAnySyncMetaInfo(folder1)); @@ -414,6 +420,7 @@ TEST_F(BraveBookmarkChangeProcessorTest, BookmarkMovedInFolder) { // BookmarkNodeMoved does not reset "last_send_time" so SendUnsynced // ignores order change untill unsynced_send_interval passes, // so here below unsynced_send_interval is 0 + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(0)); } @@ -444,6 +451,7 @@ TEST_F(BraveBookmarkChangeProcessorTest, DISABLED_MoveNodesBetweenDirs) { // Send all created objects EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", RecordsNumber(4))).Times(1); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); } @@ -466,10 +474,12 @@ TEST_F(BraveBookmarkChangeProcessorTest, DeleteFolderWithNodes) { EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", RecordsNumber(4))).Times(1); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); model()->Remove(folder1); EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS",_)).Times(1); + EXPECT_CALL(*sync_client(), ClearOrderMap()).Times(1); change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); } diff --git a/components/brave_sync/test_util.h b/components/brave_sync/test_util.h index 41cccf5628a0..e63894af871e 100644 --- a/components/brave_sync/test_util.h +++ b/components/brave_sync/test_util.h @@ -51,6 +51,7 @@ class MockBraveSyncClient : public BraveSyncClient { MOCK_METHOD1(NeedBytesFromSyncWords, void(const std::string& words)); MOCK_METHOD0(OnExtensionInitialized, void()); MOCK_METHOD0(OnSyncEnabledChanged, void()); + MOCK_METHOD0(ClearOrderMap, void()); }; std::unique_ptr CreateBraveSyncProfile(const base::FilePath& path);