From 47454639d745f30d7cbd93e030e8e77fe6c71ece Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Tue, 27 Nov 2018 18:24:27 -0800 Subject: [PATCH 1/3] Handle offline cases for sync --- components/brave_sync/BUILD.gn | 1 + .../brave_sync/brave_sync_service_impl.cc | 30 +++++++++++++++++-- .../brave_sync/brave_sync_service_impl.h | 10 +++++-- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/components/brave_sync/BUILD.gn b/components/brave_sync/BUILD.gn index 6e9184bce9e8..49497becd88c 100644 --- a/components/brave_sync/BUILD.gn +++ b/components/brave_sync/BUILD.gn @@ -30,6 +30,7 @@ source_set("js_sync_lib_impl") { "//components/pref_registry", "//content/public/browser", "//extensions/browser", + "//services/network/public/cpp", "//ui/base", ] } diff --git a/components/brave_sync/brave_sync_service_impl.cc b/components/brave_sync/brave_sync_service_impl.cc index 07855a2b3c86..e7ff376540c6 100644 --- a/components/brave_sync/brave_sync_service_impl.cc +++ b/components/brave_sync/brave_sync_service_impl.cc @@ -21,6 +21,7 @@ #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "components/bookmarks/browser/bookmark_model.h" +#include "content/public/browser/network_service_instance.h" namespace brave_sync { @@ -84,6 +85,7 @@ BraveSyncServiceImpl::BraveSyncServiceImpl(Profile* profile) : sync_prefs_.get())), timer_(std::make_unique()), unsynced_send_interval_(base::TimeDelta::FromMinutes(10)) { + content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this); // Moniter syncs prefs required in GetSettingsAndDevices profile_pref_change_registrar_.Init(profile->GetPrefs()); @@ -118,12 +120,24 @@ BraveSyncServiceImpl::BraveSyncServiceImpl(Profile* profile) : } } -BraveSyncServiceImpl::~BraveSyncServiceImpl() {} +BraveSyncServiceImpl::~BraveSyncServiceImpl() { + content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this); +} BraveSyncClient* BraveSyncServiceImpl::GetSyncClient() { return sync_client_.get(); } +void BraveSyncServiceImpl::OnConnectionChanged( + network::mojom::ConnectionType type) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + if (type == network::mojom::ConnectionType::CONNECTION_NONE) { + if (initializing_) { + OnSyncSetupError("network connection is currently unavailable"); + } + } +} + bool BraveSyncServiceImpl::IsSyncConfigured() { return sync_configured_; } @@ -142,6 +156,11 @@ void BraveSyncServiceImpl::Shutdown() { void BraveSyncServiceImpl::OnSetupSyncHaveCode(const std::string& sync_words, const std::string& device_name) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + if (content::GetNetworkConnectionTracker()->IsOffline()) { + OnSyncSetupError("network connection is currently unavailable"); + return; + } + if (sync_words.empty() || device_name.empty()) { OnSyncSetupError("missing sync words or device name"); return; @@ -167,6 +186,11 @@ void BraveSyncServiceImpl::OnSetupSyncHaveCode(const std::string& sync_words, void BraveSyncServiceImpl::OnSetupSyncNewToSync( const std::string& device_name) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + if (content::GetNetworkConnectionTracker()->IsOffline()) { + OnSyncSetupError("network connection is currently unavailable"); + return; + } + if (device_name.empty()) { OnSyncSetupError("missing device name"); return; @@ -287,9 +311,9 @@ void BraveSyncServiceImpl::OnSyncDebug(const std::string& message) { } void BraveSyncServiceImpl::OnSyncSetupError(const std::string& error) { - initializing_ = false; - if (!sync_initialized_) { + if (initializing_) { sync_prefs_->Clear(); + initializing_ = false; } NotifySyncSetupError(error); } diff --git a/components/brave_sync/brave_sync_service_impl.h b/components/brave_sync/brave_sync_service_impl.h index 4b270d9054e1..cdd56139d40c 100644 --- a/components/brave_sync/brave_sync_service_impl.h +++ b/components/brave_sync/brave_sync_service_impl.h @@ -12,6 +12,7 @@ #include "base/time/time.h" #include "brave/components/brave_sync/brave_sync_service.h" #include "brave/components/brave_sync/client/brave_sync_client.h" +#include "services/network/public/cpp/network_connection_tracker.h" #include "components/prefs/pref_change_registrar.h" FORWARD_DECLARE_TEST(BraveSyncServiceTest, BookmarkAdded); @@ -52,8 +53,10 @@ using SendDeviceSyncRecordCallback = base::OnceCallback; -class BraveSyncServiceImpl : public BraveSyncService, - public SyncMessageHandler { +class BraveSyncServiceImpl + : public BraveSyncService, + public SyncMessageHandler, + public network::NetworkConnectionTracker::NetworkConnectionObserver { public: BraveSyncServiceImpl(Profile *profile); ~BraveSyncServiceImpl() override; @@ -82,6 +85,9 @@ class BraveSyncServiceImpl : public BraveSyncService, BraveSyncClient* GetSyncClient() override; + // network::NetworkConnectionTracker::NetworkConnectionObserver: + void OnConnectionChanged(network::mojom::ConnectionType type) override; + private: FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BookmarkAdded); FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BookmarkDeleted); From 7ae1b7595f127b9b7e94016ed8abe0d9cfed47a0 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Wed, 28 Nov 2018 20:10:28 -0800 Subject: [PATCH 2/3] Adding test cases for offline scenario --- .../brave_sync/brave_sync_service_unittest.cc | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 19a01c09d57a..903edb645593 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -22,7 +22,9 @@ #include "components/bookmarks/browser/bookmark_utils.h" #include "components/bookmarks/test/test_bookmark_client.h" #include "components/prefs/pref_service.h" +#include "content/public/browser/network_service_instance.h" #include "content/public/test/test_browser_thread_bundle.h" +#include "services/network/test/test_network_connection_tracker.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -87,6 +89,8 @@ using testing::_; using testing::AtLeast; using namespace brave_sync; +using network::TestNetworkConnectionTracker; +using network::mojom::ConnectionType; class MockBraveSyncServiceObserver : public BraveSyncServiceObserver { public: @@ -97,7 +101,9 @@ class MockBraveSyncServiceObserver : public BraveSyncServiceObserver { MOCK_METHOD2(OnHaveSyncWords, void(BraveSyncService*, const std::string&)); }; -class BraveSyncServiceTest : public testing::Test { +class BraveSyncServiceTest + : public testing::Test, + public network::NetworkConnectionTracker::NetworkConnectionObserver { public: BraveSyncServiceTest() {} ~BraveSyncServiceTest() override {} @@ -128,6 +134,12 @@ class BraveSyncServiceTest : public testing::Test { observer_.reset(new MockBraveSyncServiceObserver); sync_service_->AddObserver(observer_.get()); EXPECT_TRUE(sync_service_ != NULL); + + // TestNetworkConnectionTracker::CreateInstance has been called in + // TestingBrowserProcess + TestNetworkConnectionTracker* tracker = + TestNetworkConnectionTracker::GetInstance(); + tracker->SetConnectionType(ConnectionType::CONNECTION_UNKNOWN); } void TearDown() override { @@ -136,6 +148,11 @@ class BraveSyncServiceTest : public testing::Test { profile_.reset(); } + // network::NetworkConnectionTracker::NetworkConnectionObserver: + void OnConnectionChanged(ConnectionType type) override { + wait_for_network_type_change_.QuitWhenIdle(); + } + void BookmarkAddedImpl(); Profile* profile() { return profile_.get(); } @@ -155,6 +172,8 @@ class BraveSyncServiceTest : public testing::Test { std::unique_ptr observer_; base::ScopedTempDir temp_dir_; + public: + base::RunLoop wait_for_network_type_change_; }; TEST_F(BraveSyncServiceTest, SetSyncEnabled) { @@ -244,6 +263,18 @@ TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCode) { brave_sync::prefs::kSyncEnabled)); } +TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCode_Offline) { + TestNetworkConnectionTracker* tracker = + TestNetworkConnectionTracker::GetInstance(); + tracker->SetConnectionType(ConnectionType::CONNECTION_NONE); + EXPECT_CALL(*observer(), OnSyncSetupError(sync_service(), _)); + sync_service()->OnSetupSyncHaveCode("word1 word2 word3", "test_device"); + EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( + brave_sync::prefs::kSyncEnabled)); + // Restore network connection + tracker->SetConnectionType(ConnectionType::CONNECTION_UNKNOWN); +} + TEST_F(BraveSyncServiceTest, OnSetupSyncNewToSync) { EXPECT_CALL(*sync_client(), OnSyncEnabledChanged); // Expecting sync state changed twice: for enabled state and for device name @@ -253,6 +284,33 @@ TEST_F(BraveSyncServiceTest, OnSetupSyncNewToSync) { brave_sync::prefs::kSyncEnabled)); } +TEST_F(BraveSyncServiceTest, OnSetupSyncNewToSync_Offline) { + TestNetworkConnectionTracker* tracker = + TestNetworkConnectionTracker::GetInstance(); + tracker->SetConnectionType(ConnectionType::CONNECTION_NONE); + EXPECT_CALL(*observer(), OnSyncSetupError(sync_service(), _)); + sync_service()->OnSetupSyncNewToSync("test_device"); + EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( + brave_sync::prefs::kSyncEnabled)); + // Restore network connection + tracker->SetConnectionType(ConnectionType::CONNECTION_UNKNOWN); +} + +TEST_F(BraveSyncServiceTest, OnConnectionChanged_After_OnSetupSyncNewToSync) { + content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this); + TestNetworkConnectionTracker* tracker = + TestNetworkConnectionTracker::GetInstance(); + EXPECT_CALL(*observer(), OnSyncSetupError(sync_service(), _)); + sync_service()->OnSetupSyncNewToSync("test_device"); + tracker->SetConnectionType(ConnectionType::CONNECTION_NONE); + wait_for_network_type_change_.Run(); + content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(this); + EXPECT_FALSE(profile()->GetPrefs()->GetBoolean( + brave_sync::prefs::kSyncEnabled)); + // Restore network connection + tracker->SetConnectionType(ConnectionType::CONNECTION_UNKNOWN); +} + TEST_F(BraveSyncServiceTest, GetSettingsAndDevices) { // The test absorbs OnSetupSyncNewToSync test auto callback1 = base::BindRepeating( From b58844b7a12cd81ecd6005da8cd9fc3db12cda3b Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Wed, 28 Nov 2018 20:16:44 -0800 Subject: [PATCH 3/3] TODO for https://github.com/brave/brave-core/pull/971 which is blocked by this PR --- components/brave_sync/brave_sync_service_impl.cc | 3 +++ components/brave_sync/extension/background.js | 1 + 2 files changed, 4 insertions(+) diff --git a/components/brave_sync/brave_sync_service_impl.cc b/components/brave_sync/brave_sync_service_impl.cc index e7ff376540c6..5802acf00119 100644 --- a/components/brave_sync/brave_sync_service_impl.cc +++ b/components/brave_sync/brave_sync_service_impl.cc @@ -133,6 +133,7 @@ void BraveSyncServiceImpl::OnConnectionChanged( DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if (type == network::mojom::ConnectionType::CONNECTION_NONE) { if (initializing_) { + // TODO(cezaraugusto): ERR_SYNC_NO_INTERNET in #971 OnSyncSetupError("network connection is currently unavailable"); } } @@ -157,6 +158,7 @@ void BraveSyncServiceImpl::OnSetupSyncHaveCode(const std::string& sync_words, const std::string& device_name) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if (content::GetNetworkConnectionTracker()->IsOffline()) { + // TODO(cezaraugusto): ERR_SYNC_NO_INTERNET in #971 OnSyncSetupError("network connection is currently unavailable"); return; } @@ -187,6 +189,7 @@ void BraveSyncServiceImpl::OnSetupSyncNewToSync( const std::string& device_name) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if (content::GetNetworkConnectionTracker()->IsOffline()) { + // TODO(cezaraugusto): ERR_SYNC_NO_INTERNET in #971 OnSyncSetupError("network connection is currently unavailable"); return; } diff --git a/components/brave_sync/extension/background.js b/components/brave_sync/extension/background.js index afcc65c852ac..0b974e8b822e 100644 --- a/components/brave_sync/extension/background.js +++ b/components/brave_sync/extension/background.js @@ -231,6 +231,7 @@ class InjectedObject { break; case "sync-setup-error": console.log(`"sync-setup-error" error=${JSON.stringify(arg1)}`); + // TODO(cezaraugusto): ERR_SYNC_INIT_FAILED without arg in #971 chrome.braveSync.syncSetupError(arg1/*error*/); break; case "sync-debug":