Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle offline cases for sync #973

Merged
merged 3 commits into from
Nov 29, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/brave_sync/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ source_set("js_sync_lib_impl") {
"//components/pref_registry",
"//content/public/browser",
"//extensions/browser",
"//services/network/public/cpp",
"//ui/base",
]
}
Expand Down
33 changes: 30 additions & 3 deletions components/brave_sync/brave_sync_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -84,6 +85,7 @@ BraveSyncServiceImpl::BraveSyncServiceImpl(Profile* profile) :
sync_prefs_.get())),
timer_(std::make_unique<base::RepeatingTimer>()),
unsynced_send_interval_(base::TimeDelta::FromMinutes(10)) {
content::GetNetworkConnectionTracker()->AddNetworkConnectionObserver(this);

// Moniter syncs prefs required in GetSettingsAndDevices
profile_pref_change_registrar_.Init(profile->GetPrefs());
Expand Down Expand Up @@ -118,12 +120,25 @@ 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_) {
// TODO(cezaraugusto): ERR_SYNC_NO_INTERNET in #971
OnSyncSetupError("network connection is currently unavailable");
}
}
}

bool BraveSyncServiceImpl::IsSyncConfigured() {
return sync_configured_;
}
Expand All @@ -142,6 +157,12 @@ 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()) {
// TODO(cezaraugusto): ERR_SYNC_NO_INTERNET in #971
OnSyncSetupError("network connection is currently unavailable");
return;
}

if (sync_words.empty() || device_name.empty()) {
OnSyncSetupError("missing sync words or device name");
return;
Expand All @@ -167,6 +188,12 @@ 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()) {
// TODO(cezaraugusto): ERR_SYNC_NO_INTERNET in #971
OnSyncSetupError("network connection is currently unavailable");
return;
}

if (device_name.empty()) {
OnSyncSetupError("missing device name");
return;
Expand Down Expand Up @@ -287,9 +314,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);
}
Expand Down
10 changes: 8 additions & 2 deletions components/brave_sync/brave_sync_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -52,8 +53,10 @@ using SendDeviceSyncRecordCallback = base::OnceCallback<void(const int,
const std::string&,
const std::string&)>;

class BraveSyncServiceImpl : public BraveSyncService,
public SyncMessageHandler {
class BraveSyncServiceImpl
: public BraveSyncService,
public SyncMessageHandler,
public network::NetworkConnectionTracker::NetworkConnectionObserver {
public:
BraveSyncServiceImpl(Profile *profile);
~BraveSyncServiceImpl() override;
Expand Down Expand Up @@ -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);
Expand Down
60 changes: 59 additions & 1 deletion components/brave_sync/brave_sync_service_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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:
Expand All @@ -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 {}
Expand Down Expand Up @@ -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 {
Expand All @@ -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(); }
Expand All @@ -155,6 +172,8 @@ class BraveSyncServiceTest : public testing::Test {
std::unique_ptr<MockBraveSyncServiceObserver> observer_;

base::ScopedTempDir temp_dir_;
public:
base::RunLoop wait_for_network_type_change_;
};

TEST_F(BraveSyncServiceTest, SetSyncEnabled) {
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down
1 change: 1 addition & 0 deletions components/brave_sync/extension/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand Down