From f7c25cd5ef5ea520eaa9613d4741f3a5d2ddbd06 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Tue, 18 Dec 2018 11:59:44 +0200 Subject: [PATCH 1/3] Sync: allow two delete device records --- components/brave_sync/sync_devices.cc | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/components/brave_sync/sync_devices.cc b/components/brave_sync/sync_devices.cc index 22fd1519f066..51930dbfd389 100644 --- a/components/brave_sync/sync_devices.cc +++ b/components/brave_sync/sync_devices.cc @@ -135,11 +135,6 @@ void SyncDevices::FromJson(const std::string& str_json) { void SyncDevices::Merge(const SyncDevice& device, int action, bool* actually_merged) { - /* - const int kActionCreate = 0; - const int kActionUpdate = 1; - const int kActionDelete = 2; - */ *actually_merged = false; auto existing_it = std::find_if(std::begin(devices_), std::end(devices_), @@ -147,13 +142,8 @@ void SyncDevices::Merge(const SyncDevice& device, return cur_dev.object_id_ == device.object_id_; }); - if (existing_it == std::end(devices_)) { - // TODO(bridiver) - should this be an error or a DCHECK? - } - switch (action) { case jslib_const::kActionCreate: { - //DCHECK(existing_device == nullptr); if (existing_it == std::end(devices_)) { devices_.push_back(device); *actually_merged = true; @@ -163,20 +153,19 @@ void SyncDevices::Merge(const SyncDevice& device, break; } case jslib_const::kActionUpdate: { - //DCHECK(existing_device != nullptr); DCHECK(existing_it != std::end(devices_)); - //*existing_device = device; *existing_it = device; *actually_merged = true; break; } case jslib_const::kActionDelete: { - //DCHECK(existing_device != nullptr); - DCHECK(existing_it != std::end(devices_)); - //DeleteByObjectId(device.object_id_); + // Sync js lib does not merge several DELETE records into one, + // at this point existing_it can be equal to std::end(devices_) if (existing_it != std::end(devices_)) { devices_.erase(existing_it); *actually_merged = true; + } else { + // ignoring delete, already deleted } break; } @@ -190,7 +179,6 @@ SyncDevice* SyncDevices::GetByObjectId(const std::string &object_id) { } } - //DCHECK(false) << "Not expected to find no device"; return nullptr; } @@ -201,7 +189,6 @@ const SyncDevice* SyncDevices::GetByDeviceId(const std::string &device_id) { } } - //DCHECK(false) << "Not expected to find no device"; return nullptr; } From 5073bc964357a75a24222a59dc9e8ced32c647e0 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Fri, 4 Jan 2019 17:20:53 +0200 Subject: [PATCH 2/3] Sync: unit test for allow two delete device records --- components/brave_sync/brave_sync_service_unittest.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 9264554a66ae..bde74133b252 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -466,6 +466,13 @@ TEST_F(BraveSyncServiceTest, OnDeleteDevice) { auto resolved_record = SyncRecord::Clone(*records.at(2)); resolved_record->action = SyncRecord::Action::A_DELETE; resolved_records.push_back(std::move(resolved_record)); + + // Emulate we have twice the same delete device record to ensure we + // don't have failed DCHECK in debug build + auto resolved_record2 = SyncRecord::Clone(*records.at(2)); + resolved_record2->action = jslib::SyncRecord::Action::A_DELETE; + resolved_records.push_back(std::move(resolved_record2)); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); sync_service()->OnResolvedPreferences(resolved_records); From b74cb87ad8d135f1c06735958e07bd2950b100cd Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Thu, 14 Feb 2019 13:56:27 +0200 Subject: [PATCH 3/3] Lint fixes --- .../brave_sync/brave_sync_service_unittest.cc | 2 +- components/brave_sync/sync_devices.cc | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index bde74133b252..91caabdddb84 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -470,7 +470,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDevice) { // Emulate we have twice the same delete device record to ensure we // don't have failed DCHECK in debug build auto resolved_record2 = SyncRecord::Clone(*records.at(2)); - resolved_record2->action = jslib::SyncRecord::Action::A_DELETE; + resolved_record2->action = SyncRecord::Action::A_DELETE; resolved_records.push_back(std::move(resolved_record2)); EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); diff --git a/components/brave_sync/sync_devices.cc b/components/brave_sync/sync_devices.cc index 51930dbfd389..e0c842a1d545 100644 --- a/components/brave_sync/sync_devices.cc +++ b/components/brave_sync/sync_devices.cc @@ -1,9 +1,12 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public +/* Copyright 2016 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ #include "brave/components/brave_sync/sync_devices.h" +#include + #include "base/json/json_writer.h" #include "base/json/json_reader.h" #include "base/logging.h" @@ -89,7 +92,7 @@ void SyncDevices::FromJson(const std::string& str_json) { return; } - //JSON ==> Value + // JSON ==> Value int error_code_out = 0; std::string error_msg_out; int error_line_out = 0; @@ -110,7 +113,7 @@ void SyncDevices::FromJson(const std::string& str_json) { devices_.clear(); const base::Value* pv_arr = records_v->FindKey("devices"); CHECK(pv_arr->is_list()); - for (const base::Value &val : pv_arr->GetList() ) { + for (const base::Value &val : pv_arr->GetList()) { std::string name = val.FindKey("name")->GetString(); std::string object_id = val.FindKey("object_id")->GetString(); std::string device_id = val.FindKey("device_id")->GetString(); @@ -126,10 +129,8 @@ void SyncDevices::FromJson(const std::string& str_json) { name, object_id, device_id, - last_active - )); + last_active) ); } - } void SyncDevices::Merge(const SyncDevice& device, @@ -173,7 +174,7 @@ void SyncDevices::Merge(const SyncDevice& device, } SyncDevice* SyncDevices::GetByObjectId(const std::string &object_id) { - for (auto & device: devices_) { + for (auto& device : devices_) { if (device.object_id_ == object_id) { return &device; } @@ -183,7 +184,7 @@ SyncDevice* SyncDevices::GetByObjectId(const std::string &object_id) { } const SyncDevice* SyncDevices::GetByDeviceId(const std::string &device_id) { - for (const auto & device: devices_) { + for (const auto& device : devices_) { if (device.device_id_ == device_id) { return &device; } @@ -208,4 +209,4 @@ void SyncDevices::DeleteByObjectId(const std::string &object_id) { } } -} // namespace brave_sync +} // namespace brave_sync