From 5be8b586be828f6c54d153f74a5af39c96645377 Mon Sep 17 00:00:00 2001 From: skau Date: Wed, 8 Feb 2017 22:07:58 -0800 Subject: [PATCH] Sync Printer information for Chrome OS. Move printer preference storage from user preference JSON to LevelDB via ModelTypeStore. Enable User Sync Service for the syncer::PRINTERS type to synchronize printer configuration information between devices. BUG=616862 Review-Url: https://codereview.chromium.org/2656813004 Cr-Commit-Position: refs/heads/master@{#449216} --- chrome/browser/chromeos/BUILD.gn | 2 + chrome/browser/chromeos/printing/DEPS | 5 + .../chromeos/printing/printer_pref_manager.cc | 136 ++++--- .../chromeos/printing/printer_pref_manager.h | 33 +- .../printing/printer_pref_manager_factory.cc | 24 +- .../printing/printer_pref_manager_unittest.cc | 169 ++++++--- .../chromeos/printing/printers_sync_bridge.cc | 358 ++++++++++++++++++ .../chromeos/printing/printers_sync_bridge.h | 73 ++++ .../printing/specifics_translation.cc | 25 +- .../chromeos/printing/specifics_translation.h | 4 + chrome/browser/sync/chrome_sync_client.cc | 10 + .../profile_sync_service_factory_unittest.cc | 8 +- .../sync/test/integration/printers_helper.cc | 161 ++++++++ .../sync/test/integration/printers_helper.h | 75 ++++ .../single_client_printers_sync_test.cc | 89 +++++ .../two_client_printers_sync_test.cc | 150 ++++++++ chrome/test/BUILD.gn | 8 + chromeos/printing/printer_configuration.h | 2 +- .../profile_sync_components_factory_impl.cc | 8 + 19 files changed, 1207 insertions(+), 133 deletions(-) create mode 100644 chrome/browser/chromeos/printing/DEPS create mode 100644 chrome/browser/chromeos/printing/printers_sync_bridge.cc create mode 100644 chrome/browser/chromeos/printing/printers_sync_bridge.h create mode 100644 chrome/browser/sync/test/integration/printers_helper.cc create mode 100644 chrome/browser/sync/test/integration/printers_helper.h create mode 100644 chrome/browser/sync/test/integration/single_client_printers_sync_test.cc create mode 100644 chrome/browser/sync/test/integration/two_client_printers_sync_test.cc diff --git a/chrome/browser/chromeos/BUILD.gn b/chrome/browser/chromeos/BUILD.gn index c206c0ff7fe496..6d382dd7b4f9a3 100644 --- a/chrome/browser/chromeos/BUILD.gn +++ b/chrome/browser/chromeos/BUILD.gn @@ -1196,6 +1196,8 @@ source_set("chromeos") { "printing/printer_pref_manager.h", "printing/printer_pref_manager_factory.cc", "printing/printer_pref_manager_factory.h", + "printing/printers_sync_bridge.cc", + "printing/printers_sync_bridge.h", "printing/specifics_translation.cc", "printing/specifics_translation.h", "profiles/avatar_menu_actions_chromeos.cc", diff --git a/chrome/browser/chromeos/printing/DEPS b/chrome/browser/chromeos/printing/DEPS new file mode 100644 index 00000000000000..84c882c2b40a26 --- /dev/null +++ b/chrome/browser/chromeos/printing/DEPS @@ -0,0 +1,5 @@ +include_rules = [ + "+components/sync/base", + "+components/sync/model", + "+components/sync/protocol", +] diff --git a/chrome/browser/chromeos/printing/printer_pref_manager.cc b/chrome/browser/chromeos/printing/printer_pref_manager.cc index 3e52bf1ea9e5f0..748fe569320a0a 100644 --- a/chrome/browser/chromeos/printing/printer_pref_manager.cc +++ b/chrome/browser/chromeos/printing/printer_pref_manager.cc @@ -13,61 +13,48 @@ #include "base/json/json_reader.h" #include "base/md5.h" #include "base/memory/ptr_util.h" +#include "base/optional.h" #include "base/values.h" +#include "chrome/browser/chromeos/printing/printers_sync_bridge.h" +#include "chrome/browser/chromeos/printing/specifics_translation.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/pref_names.h" #include "chromeos/printing/printer_configuration.h" #include "chromeos/printing/printer_translator.h" #include "components/pref_registry/pref_registry_syncable.h" -#include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" -#include "components/prefs/scoped_user_pref_update.h" namespace chromeos { namespace { -const base::ListValue* GetPrinterList(Profile* profile) { - return profile->GetPrefs()->GetList(prefs::kPrintingDevices); -} - -// Returns the printer with the matching |id| from the list |values|. The -// returned value is mutable and |values| could be modified. -base::DictionaryValue* FindPrinterPref(const base::ListValue* values, - const std::string& id) { - for (const auto& value : *values) { - base::DictionaryValue* printer_dictionary; - if (!value->GetAsDictionary(&printer_dictionary)) - continue; - - std::string printer_id; - if (printer_dictionary->GetString(printing::kPrinterId, &printer_id) && - id == printer_id) - return printer_dictionary; +// Adds |printer| with |id| to prefs. Returns true if the printer is new, +// false for an update. +bool UpdatePrinterPref(PrintersSyncBridge* sync_bridge, + const std::string& id, + const Printer& printer) { + base::Optional specifics = + sync_bridge->GetPrinter(id); + if (!specifics.has_value()) { + sync_bridge->AddPrinter(printing::PrinterToSpecifics(printer)); + return true; } - return nullptr; -} + // Preserve fields in the proto which we don't understand. + std::unique_ptr updated_printer = + base::MakeUnique(*specifics); + printing::MergePrinterToSpecifics(printer, updated_printer.get()); + sync_bridge->AddPrinter(std::move(updated_printer)); -void UpdatePrinterPref( - Profile* profile, - const std::string& id, - std::unique_ptr printer_dictionary) { - ListPrefUpdate update(profile->GetPrefs(), prefs::kPrintingDevices); - base::ListValue* printer_list = update.Get(); - DCHECK(printer_list) << "Register the printer preference"; - base::DictionaryValue* printer = FindPrinterPref(printer_list, id); - if (!printer) { - printer_list->Append(std::move(printer_dictionary)); - return; - } - - printer->MergeDictionary(printer_dictionary.get()); + return false; } } // anonymous namespace -PrinterPrefManager::PrinterPrefManager(Profile* profile) : profile_(profile) { +PrinterPrefManager::PrinterPrefManager( + Profile* profile, + std::unique_ptr sync_bridge) + : profile_(profile), sync_bridge_(std::move(sync_bridge)) { pref_change_registrar_.Init(profile->GetPrefs()); pref_change_registrar_.Add( prefs::kRecommendedNativePrinters, @@ -81,27 +68,18 @@ PrinterPrefManager::~PrinterPrefManager() {} // static void PrinterPrefManager::RegisterProfilePrefs( user_prefs::PrefRegistrySyncable* registry) { - // TODO(skau): Change to user_prefs::PrefRegistrySyncable::SYNCABLE_PREF) when - // sync is implemented. registry->RegisterListPref(prefs::kPrintingDevices, - PrefRegistry::NO_REGISTRATION_FLAGS); + user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); registry->RegisterListPref(prefs::kRecommendedNativePrinters); } std::vector> PrinterPrefManager::GetPrinters() const { std::vector> printers; - const base::ListValue* values = GetPrinterList(profile_); - for (const auto& value : *values) { - const base::DictionaryValue* printer_dictionary = nullptr; - value->GetAsDictionary(&printer_dictionary); - - DCHECK(printer_dictionary); - - std::unique_ptr printer = - printing::PrefToPrinter(*printer_dictionary); - if (printer) - printers.push_back(std::move(printer)); + std::vector values = + sync_bridge_->GetAllPrinters(); + for (const auto& value : values) { + printers.push_back(printing::SpecificsToPrinter(value)); } return printers; @@ -133,30 +111,62 @@ std::unique_ptr PrinterPrefManager::GetPrinter( if (found != policy_printers.end()) return printing::RecommendedPrinterToPrinter(*(found->second)); - const base::ListValue* values = GetPrinterList(profile_); - const base::DictionaryValue* printer = FindPrinterPref(values, printer_id); - - return printer ? printing::PrefToPrinter(*printer) : nullptr; + base::Optional printer = + sync_bridge_->GetPrinter(printer_id); + return printer.has_value() ? printing::SpecificsToPrinter(*printer) : nullptr; } void PrinterPrefManager::RegisterPrinter(std::unique_ptr printer) { - if (printer->id().empty()) + if (printer->id().empty()) { printer->set_id(base::GenerateGUID()); + } DCHECK_EQ(Printer::SRC_USER_PREFS, printer->source()); - std::unique_ptr updated_printer = - printing::PrinterToPref(*printer); - UpdatePrinterPref(profile_, printer->id(), std::move(updated_printer)); + bool new_printer = + UpdatePrinterPref(sync_bridge_.get(), printer->id(), *printer); + + if (new_printer) { + for (Observer& obs : observers_) { + obs.OnPrinterAdded(*printer); + } + } else { + for (Observer& obs : observers_) { + obs.OnPrinterUpdated(*printer); + } + } } bool PrinterPrefManager::RemovePrinter(const std::string& printer_id) { DCHECK(!printer_id.empty()); - ListPrefUpdate update(profile_->GetPrefs(), prefs::kPrintingDevices); - base::ListValue* printer_list = update.Get(); - DCHECK(printer_list) << "Printer preference not registered"; - base::DictionaryValue* printer = FindPrinterPref(printer_list, printer_id); - return printer && printer_list->Remove(*printer, nullptr); + base::Optional printer = + sync_bridge_->GetPrinter(printer_id); + bool success = false; + if (printer.has_value()) { + std::unique_ptr p = printing::SpecificsToPrinter(*printer); + success = sync_bridge_->RemovePrinter(p->id()); + if (success) { + for (Observer& obs : observers_) { + obs.OnPrinterRemoved(*p); + } + } + } else { + LOG(WARNING) << "Could not find printer" << printer_id; + } + + return success; +} + +void PrinterPrefManager::AddObserver(Observer* observer) { + observers_.AddObserver(observer); +} + +void PrinterPrefManager::RemoveObserver(Observer* observer) { + observers_.RemoveObserver(observer); +} + +PrintersSyncBridge* PrinterPrefManager::GetSyncBridge() { + return sync_bridge_.get(); } // This method is not thread safe and could interact poorly with readers of diff --git a/chrome/browser/chromeos/printing/printer_pref_manager.h b/chrome/browser/chromeos/printing/printer_pref_manager.h index 222de0d1e279de..867ac737ea1eca 100644 --- a/chrome/browser/chromeos/printing/printer_pref_manager.h +++ b/chrome/browser/chromeos/printing/printer_pref_manager.h @@ -10,6 +10,9 @@ #include #include +#include "base/memory/weak_ptr.h" +#include "base/observer_list.h" +#include "chrome/browser/chromeos/printing/printers_sync_bridge.h" #include "chromeos/printing/printer_configuration.h" #include "chromeos/printing/printer_translator.h" #include "components/keyed_service/core/keyed_service.h" @@ -25,7 +28,15 @@ namespace chromeos { class PrinterPrefManager : public KeyedService { public: - explicit PrinterPrefManager(Profile* profile); + class Observer { + public: + virtual void OnPrinterAdded(const Printer& printer) = 0; + virtual void OnPrinterUpdated(const Printer& printer) = 0; + virtual void OnPrinterRemoved(const Printer& printer) = 0; + }; + + PrinterPrefManager(Profile* profile, + std::unique_ptr sync_bridge); ~PrinterPrefManager() override; // Register the printing preferences with the |registry|. @@ -48,6 +59,19 @@ class PrinterPrefManager : public KeyedService { // the printer was successfully removed. bool RemovePrinter(const std::string& printer_id); + // Attach |observer| for notification of events. |observer| is expected to + // live on the same thread (UI) as this object. OnPrinter* methods are + // invoked inline so calling RegisterPrinter in response to OnPrinterAdded is + // forbidden. + void AddObserver(PrinterPrefManager::Observer* observer); + + // Remove |observer| so that it no longer receives notifications. After the + // completion of this method, the |observer| can be safely destroyed. + void RemoveObserver(PrinterPrefManager::Observer* observer); + + // Returns a ModelTypeSyncBridge for the sync client. + PrintersSyncBridge* GetSyncBridge(); + private: // Updates the in-memory recommended printer list. void UpdateRecommendedPrinters(); @@ -55,11 +79,18 @@ class PrinterPrefManager : public KeyedService { Profile* profile_; PrefChangeRegistrar pref_change_registrar_; + // The backend for profile printers. + std::unique_ptr sync_bridge_; + // Contains the keys for all recommended printers in order so we can return // the list of recommended printers in the order they were received. std::vector recommended_printer_ids_; std::map> recommended_printers_; + + base::ObserverList observers_; + + DISALLOW_COPY_AND_ASSIGN(PrinterPrefManager); }; } // namespace chromeos diff --git a/chrome/browser/chromeos/printing/printer_pref_manager_factory.cc b/chrome/browser/chromeos/printing/printer_pref_manager_factory.cc index 7baed0e10c8e2c..f8f0078ea05659 100644 --- a/chrome/browser/chromeos/printing/printer_pref_manager_factory.cc +++ b/chrome/browser/chromeos/printing/printer_pref_manager_factory.cc @@ -4,8 +4,16 @@ #include "chrome/browser/chromeos/printing/printer_pref_manager_factory.h" +#include +#include + +#include "base/debug/dump_without_crashing.h" +#include "base/memory/ptr_util.h" +#include "chrome/browser/chromeos/printing/printers_sync_bridge.h" #include "chrome/browser/profiles/incognito_helpers.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/profile_sync_service_factory.h" +#include "components/browser_sync/profile_sync_service.h" #include "components/keyed_service/content/browser_context_dependency_manager.h" #include "content/public/browser/browser_context.h" @@ -45,7 +53,21 @@ PrinterPrefManagerFactory::~PrinterPrefManagerFactory() {} PrinterPrefManager* PrinterPrefManagerFactory::BuildServiceInstanceFor( content::BrowserContext* browser_context) const { Profile* profile = Profile::FromBrowserContext(browser_context); - return new PrinterPrefManager(profile); + + browser_sync::ProfileSyncService* sync_service = + ProfileSyncServiceFactory::GetForProfile(profile); + + // TODO(skau): --disable-sync and --enable-native-cups are mutually exclusive + // until crbug.com/688533 is resolved. + DCHECK(sync_service); + + std::unique_ptr sync_bridge = + base::MakeUnique( + sync_service->GetModelTypeStoreFactory(syncer::PRINTERS), + base::BindRepeating( + base::IgnoreResult(&base::debug::DumpWithoutCrashing))); + + return new PrinterPrefManager(profile, std::move(sync_bridge)); } } // namespace chromeos diff --git a/chrome/browser/chromeos/printing/printer_pref_manager_unittest.cc b/chrome/browser/chromeos/printing/printer_pref_manager_unittest.cc index 11ec115b51de8d..ea3bcad0d1389a 100644 --- a/chrome/browser/chromeos/printing/printer_pref_manager_unittest.cc +++ b/chrome/browser/chromeos/printing/printer_pref_manager_unittest.cc @@ -7,10 +7,17 @@ #include #include +#include "base/bind.h" +#include "base/debug/dump_without_crashing.h" #include "base/memory/ptr_util.h" +#include "base/optional.h" +#include "base/run_loop.h" #include "chrome/browser/chromeos/printing/printer_pref_manager_factory.h" +#include "chrome/browser/chromeos/printing/printers_sync_bridge.h" #include "chrome/common/pref_names.h" #include "chrome/test/base/testing_profile.h" +#include "components/sync/model/fake_model_type_change_processor.h" +#include "components/sync/model/model_type_store.h" #include "components/sync_preferences/testing_pref_service_syncable.h" #include "content/public/test/test_browser_thread_bundle.h" #include "testing/gtest/include/gtest/gtest.h" @@ -34,72 +41,138 @@ const char kLexJson[] = R"json({ }, } )json"; +// Helper class to record observed events. +class LoggingObserver : public PrinterPrefManager::Observer { + public: + void OnPrinterAdded(const Printer& printer) override { + last_added_ = printer; + } + + void OnPrinterUpdated(const Printer& printer) override { + last_updated_ = printer; + } + + void OnPrinterRemoved(const Printer& printer) override { + last_removed_ = printer; + } + + // Returns true if OnPrinterAdded was called. + bool AddCalled() const { return last_added_.has_value(); } + + // Returns true if OnPrinterUpdated was called. + bool UpdateCalled() const { return last_updated_.has_value(); } + + // Returns true if OnPrinterRemoved was called. + bool RemoveCalled() const { return last_removed_.has_value(); } + + // Returns the last printer that was added. If AddCalled is false, there will + // be no printer. + base::Optional LastAdded() const { return last_added_; } + // Returns the last printer that was updated. If UpdateCalled is false, there + // will be no printer. + base::Optional LastUpdated() const { return last_updated_; } + // Returns the last printer that was removed. If RemoveCalled is false, there + // will be no printer. + base::Optional LastRemoved() const { return last_removed_; } + + private: + base::Optional last_added_; + base::Optional last_updated_; + base::Optional last_removed_; +}; + } // namespace -TEST(PrinterPrefManagerTest, AddPrinter) { - content::TestBrowserThreadBundle thread_bundle; - auto profile = base::MakeUnique(); - PrinterPrefManager* manager = - PrinterPrefManagerFactory::GetForBrowserContext(profile.get()); +class PrinterPrefManagerTest : public testing::Test { + protected: + PrinterPrefManagerTest() : profile_(base::MakeUnique()) { + thread_bundle_ = base::MakeUnique(); + + auto sync_bridge = base::MakeUnique( + base::Bind(&syncer::ModelTypeStore::CreateInMemoryStoreForTest, + syncer::PRINTERS), + base::BindRepeating( + base::IgnoreResult(&base::debug::DumpWithoutCrashing))); + + manager_ = base::MakeUnique(profile_.get(), + std::move(sync_bridge)); + + base::RunLoop().RunUntilIdle(); + } + + ~PrinterPrefManagerTest() override { + manager_.reset(); - manager->RegisterPrinter(base::MakeUnique(kPrinterId)); + // Explicitly release the profile before the thread_bundle. Otherwise, the + // profile destructor throws an error. + profile_.reset(); + thread_bundle_.reset(); + } - auto printers = manager->GetPrinters(); + std::unique_ptr profile_; + std::unique_ptr manager_; + + private: + std::unique_ptr thread_bundle_; +}; + +TEST_F(PrinterPrefManagerTest, AddPrinter) { + LoggingObserver observer; + manager_->AddObserver(&observer); + manager_->RegisterPrinter(base::MakeUnique(kPrinterId)); + + auto printers = manager_->GetPrinters(); ASSERT_EQ(1U, printers.size()); EXPECT_EQ(kPrinterId, printers[0]->id()); EXPECT_EQ(Printer::Source::SRC_USER_PREFS, printers[0]->source()); -} -TEST(PrinterPrefManagerTest, UpdatePrinterAssignsId) { - content::TestBrowserThreadBundle thread_bundle; - auto profile = base::MakeUnique(); - PrinterPrefManager* manager = - PrinterPrefManagerFactory::GetForBrowserContext(profile.get()); + EXPECT_TRUE(observer.AddCalled()); + EXPECT_FALSE(observer.UpdateCalled()); +} - manager->RegisterPrinter(base::MakeUnique()); +TEST_F(PrinterPrefManagerTest, UpdatePrinterAssignsId) { + manager_->RegisterPrinter(base::MakeUnique()); - auto printers = manager->GetPrinters(); + auto printers = manager_->GetPrinters(); ASSERT_EQ(1U, printers.size()); EXPECT_FALSE(printers[0]->id().empty()); } -TEST(PrinterPrefManagerTest, UpdatePrinter) { - content::TestBrowserThreadBundle thread_bundle; - auto profile = base::MakeUnique(); - PrinterPrefManager* manager = - PrinterPrefManagerFactory::GetForBrowserContext(profile.get()); - - manager->RegisterPrinter(base::MakeUnique(kPrinterId)); +TEST_F(PrinterPrefManagerTest, UpdatePrinter) { + manager_->RegisterPrinter(base::MakeUnique(kPrinterId)); auto updated_printer = base::MakeUnique(kPrinterId); updated_printer->set_uri(kUri); - manager->RegisterPrinter(std::move(updated_printer)); - auto printers = manager->GetPrinters(); + // Register observer so it only receives the update event. + LoggingObserver observer; + manager_->AddObserver(&observer); + + manager_->RegisterPrinter(std::move(updated_printer)); + + auto printers = manager_->GetPrinters(); ASSERT_EQ(1U, printers.size()); EXPECT_EQ(kUri, printers[0]->uri()); -} -TEST(PrinterPrefManagerTest, RemovePrinter) { - content::TestBrowserThreadBundle thread_bundle; - auto profile = base::MakeUnique(); - PrinterPrefManager* manager = - PrinterPrefManagerFactory::GetForBrowserContext(profile.get()); + EXPECT_TRUE(observer.UpdateCalled()); + EXPECT_FALSE(observer.AddCalled()); +} - manager->RegisterPrinter(base::MakeUnique("OtherUUID")); - manager->RegisterPrinter(base::MakeUnique(kPrinterId)); - manager->RegisterPrinter(base::MakeUnique()); +TEST_F(PrinterPrefManagerTest, RemovePrinter) { + manager_->RegisterPrinter(base::MakeUnique("OtherUUID")); + manager_->RegisterPrinter(base::MakeUnique(kPrinterId)); + manager_->RegisterPrinter(base::MakeUnique()); - manager->RemovePrinter(kPrinterId); + manager_->RemovePrinter(kPrinterId); - auto printers = manager->GetPrinters(); + auto printers = manager_->GetPrinters(); ASSERT_EQ(2U, printers.size()); EXPECT_NE(kPrinterId, printers.at(0)->id()); EXPECT_NE(kPrinterId, printers.at(1)->id()); } -TEST(PrinterPrefManagerTest, RecommendedPrinters) { - content::TestBrowserThreadBundle thread_bundle; +// Tests for policy printers +TEST_F(PrinterPrefManagerTest, RecommendedPrinters) { std::string first_printer = R"json({ "display_name": "Color Laser", @@ -120,42 +193,32 @@ TEST(PrinterPrefManagerTest, RecommendedPrinters) { value->AppendString(first_printer); value->AppendString(second_printer); - TestingProfile profile; sync_preferences::TestingPrefServiceSyncable* prefs = - profile.GetTestingPrefService(); + profile_->GetTestingPrefService(); // TestingPrefSyncableService assumes ownership of |value|. prefs->SetManagedPref(prefs::kRecommendedNativePrinters, value.release()); - PrinterPrefManager* manager = - PrinterPrefManagerFactory::GetForBrowserContext(&profile); - - auto printers = manager->GetRecommendedPrinters(); + auto printers = manager_->GetRecommendedPrinters(); ASSERT_EQ(2U, printers.size()); EXPECT_EQ("Color Laser", printers[0]->display_name()); EXPECT_EQ("ipp://192.168.1.5", printers[1]->uri()); EXPECT_EQ(Printer::Source::SRC_POLICY, printers[1]->source()); } -TEST(PrinterPrefManagerTest, GetRecommendedPrinter) { - content::TestBrowserThreadBundle thread_bundle; - +TEST_F(PrinterPrefManagerTest, GetRecommendedPrinter) { std::string printer = kLexJson; auto value = base::MakeUnique(); value->AppendString(printer); - TestingProfile profile; sync_preferences::TestingPrefServiceSyncable* prefs = - profile.GetTestingPrefService(); + profile_->GetTestingPrefService(); // TestingPrefSyncableService assumes ownership of |value|. prefs->SetManagedPref(prefs::kRecommendedNativePrinters, value.release()); - PrinterPrefManager* manager = - PrinterPrefManagerFactory::GetForBrowserContext(&profile); - - auto printers = manager->GetRecommendedPrinters(); + auto printers = manager_->GetRecommendedPrinters(); const Printer& from_list = *(printers.front()); - std::unique_ptr retrieved = manager->GetPrinter(from_list.id()); + std::unique_ptr retrieved = manager_->GetPrinter(from_list.id()); EXPECT_EQ(from_list.id(), retrieved->id()); EXPECT_EQ("LexaPrint", from_list.display_name()); diff --git a/chrome/browser/chromeos/printing/printers_sync_bridge.cc b/chrome/browser/chromeos/printing/printers_sync_bridge.cc new file mode 100644 index 00000000000000..3fc5cc2c8655f0 --- /dev/null +++ b/chrome/browser/chromeos/printing/printers_sync_bridge.cc @@ -0,0 +1,358 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/chromeos/printing/printers_sync_bridge.h" + +#include +#include +#include +#include +#include + +#include "base/bind.h" +#include "base/memory/ptr_util.h" +#include "base/optional.h" +#include "base/stl_util.h" +#include "components/sync/base/report_unrecoverable_error.h" +#include "components/sync/model/model_type_change_processor.h" +#include "components/sync/model/model_type_store.h" +#include "components/sync/model/mutable_data_batch.h" +#include "components/sync/protocol/model_type_state.pb.h" +#include "components/sync/protocol/sync.pb.h" + +namespace chromeos { + +namespace { + +using Result = syncer::ModelTypeStore::Result; + +using syncer::ConflictResolution; +using syncer::EntityChange; +using syncer::EntityChangeList; +using syncer::EntityData; +using syncer::ModelTypeChangeProcessor; +using syncer::ModelTypeStore; +using syncer::MetadataChangeList; + +std::unique_ptr CopyToEntityData( + const sync_pb::PrinterSpecifics& specifics) { + auto entity_data = base::MakeUnique(); + *entity_data->specifics.mutable_printer() = specifics; + entity_data->non_unique_name = + specifics.display_name().empty() ? "PRINTER" : specifics.display_name(); + return entity_data; +} + +// Store |specifics| locally in |data| and record change in |batch|. +void StoreSpecifics( + std::unique_ptr specifics, + std::map>* data, + ModelTypeStore::WriteBatch* batch) { + const std::string id = specifics->id(); + batch->WriteData(id, specifics->SerializeAsString()); + (*data)[id] = std::move(specifics); +} + +// Remove the specific with |id| from |data| and update |batch| with the change. +bool DeleteSpecifics( + const std::string& id, + std::map>* data, + ModelTypeStore::WriteBatch* batch) { + auto iter = data->find(id); + if (iter != data->end()) { + batch->DeleteData(id); + data->erase(iter); + return true; + } + + return false; +} + +} // namespace + +// Delegate class which helps to manage the ModelTypeStore. +class PrintersSyncBridge::StoreProxy { + public: + StoreProxy(PrintersSyncBridge* owner, + const syncer::ModelTypeStoreFactory& callback) + : owner_(owner), weak_ptr_factory_(this) { + callback.Run(base::Bind(&StoreProxy::OnStoreCreated, + weak_ptr_factory_.GetWeakPtr())); + } + + // Returns true if the store has been initialized. + bool Ready() { return store_.get() != nullptr; } + + // Returns a new WriteBatch. + std::unique_ptr CreateWriteBatch() { + CHECK(store_); + return store_->CreateWriteBatch(); + } + + // Commits writes to the database and updates metadata. + void Commit(std::unique_ptr batch) { + CHECK(store_); + store_->CommitWriteBatch( + std::move(batch), + base::Bind(&StoreProxy::OnCommit, weak_ptr_factory_.GetWeakPtr())); + } + + private: + // Callback for ModelTypeStore initialization. + void OnStoreCreated(Result result, std::unique_ptr store) { + if (result == Result::SUCCESS) { + store_ = std::move(store); + store_->ReadAllData(base::Bind(&StoreProxy::OnReadAllData, + weak_ptr_factory_.GetWeakPtr())); + } else { + owner_->change_processor()->ReportError( + FROM_HERE, "ModelTypeStore creation failed."); + } + } + + void OnReadAllData(Result result, + std::unique_ptr record_list) { + if (result != Result::SUCCESS) { + owner_->change_processor()->ReportError(FROM_HERE, + "Initial load of data failed"); + return; + } + + bool error = false; + for (const ModelTypeStore::Record& r : *record_list) { + auto specifics = base::MakeUnique(); + if (specifics->ParseFromString(r.value)) { + owner_->all_data_[specifics->id()] = std::move(specifics); + } else { + error = true; + } + } + + if (error) { + owner_->change_processor()->ReportError( + FROM_HERE, "Failed to deserialize all specifics."); + return; + } + + // Data loaded. Load metadata. + store_->ReadAllMetadata(base::Bind(&StoreProxy::OnReadAllMetadata, + weak_ptr_factory_.GetWeakPtr())); + } + + // Callback to handle commit errors. + void OnCommit(ModelTypeStore::Result result) { + if (result != Result::SUCCESS) { + LOG(WARNING) << "Failed to commit operation to store"; + owner_->change_processor()->ReportError(FROM_HERE, + "Failed to commit to store"); + } + } + + void OnReadAllMetadata( + base::Optional error, + std::unique_ptr metadata_batch) { + if (error) { + owner_->change_processor()->ReportError(error.value()); + return; + } + + owner_->change_processor()->ModelReadyToSync(std::move(metadata_batch)); + } + + PrintersSyncBridge* owner_; + + std::unique_ptr store_; + base::WeakPtrFactory weak_ptr_factory_; +}; + +PrintersSyncBridge::PrintersSyncBridge( + const syncer::ModelTypeStoreFactory& callback, + const base::RepeatingClosure& error_callback) + : ModelTypeSyncBridge(base::BindRepeating(&ModelTypeChangeProcessor::Create, + error_callback), + syncer::PRINTERS), + store_delegate_(base::MakeUnique(this, callback)) {} + +PrintersSyncBridge::~PrintersSyncBridge() {} + +std::unique_ptr +PrintersSyncBridge::CreateMetadataChangeList() { + return ModelTypeStore::WriteBatch::CreateMetadataChangeList(); +} + +base::Optional PrintersSyncBridge::MergeSyncData( + std::unique_ptr metadata_change_list, + syncer::EntityDataMap entity_data_map) { + DCHECK(change_processor()->IsTrackingMetadata()); + + std::unique_ptr batch = + store_delegate_->CreateWriteBatch(); + for (const auto& entry : entity_data_map) { + const sync_pb::PrinterSpecifics& specifics = + entry.second.value().specifics.printer(); + + DCHECK_EQ(entry.first, specifics.id()); + + // Write the update to local storage even if we already have it. + StoreSpecifics(base::MakeUnique(specifics), + &all_data_, batch.get()); + } + + // Inform the change processor of the new local entities and generate + // appropriate metadata. + for (const auto& entry : all_data_) { + const std::string& local_entity_id = entry.first; + if (!base::ContainsKey(entity_data_map, local_entity_id)) { + // Only local objects which were not updated are uploaded. Objects for + // which there was a remote copy are overwritten. + change_processor()->Put(local_entity_id, CopyToEntityData(*entry.second), + metadata_change_list.get()); + } + } + + batch->TransferMetadataChanges(std::move(metadata_change_list)); + store_delegate_->Commit(std::move(batch)); + return {}; +} + +base::Optional PrintersSyncBridge::ApplySyncChanges( + std::unique_ptr metadata_change_list, + EntityChangeList entity_changes) { + std::unique_ptr batch = + store_delegate_->CreateWriteBatch(); + // For all the entities from the server, apply changes. + for (const EntityChange& change : entity_changes) { + // We register the entity's storage key as our printer ids since they're + // globally unique. + const std::string& id = change.storage_key(); + if (change.type() == EntityChange::ACTION_DELETE) { + // Server says delete, try to remove locally. + DeleteSpecifics(id, &all_data_, batch.get()); + } else { + // Server says update, overwrite whatever is local. Conflict resolution + // guarantees that this will be the newest version of the object. + const sync_pb::PrinterSpecifics& specifics = + change.data().specifics.printer(); + DCHECK_EQ(id, specifics.id()); + StoreSpecifics(base::MakeUnique(specifics), + &all_data_, batch.get()); + } + } + + // Update the local database with metadata for the incoming changes. + batch->TransferMetadataChanges(std::move(metadata_change_list)); + + store_delegate_->Commit(std::move(batch)); + return {}; +} + +void PrintersSyncBridge::GetData(StorageKeyList storage_keys, + DataCallback callback) { + auto batch = base::MakeUnique(); + for (const auto& key : storage_keys) { + auto found = all_data_.find(key); + if (found != all_data_.end()) { + batch->Put(key, CopyToEntityData(*found->second)); + } + } + + callback.Run(std::move(batch)); +} + +void PrintersSyncBridge::GetAllData(DataCallback callback) { + auto batch = base::MakeUnique(); + for (const auto& entry : all_data_) { + batch->Put(entry.first, CopyToEntityData(*entry.second)); + } + + callback.Run(std::move(batch)); +} + +std::string PrintersSyncBridge::GetClientTag(const EntityData& entity_data) { + // Printers were never synced prior to USS so this can match GetStorageKey. + return GetStorageKey(entity_data); +} + +std::string PrintersSyncBridge::GetStorageKey(const EntityData& entity_data) { + DCHECK(entity_data.specifics.has_printer()); + return entity_data.specifics.printer().id(); +} + +// Picks the entity with the most recent updated time as the canonical version. +ConflictResolution PrintersSyncBridge::ResolveConflict( + const EntityData& local_data, + const EntityData& remote_data) const { + DCHECK(local_data.specifics.has_printer()); + DCHECK(remote_data.specifics.has_printer()); + + const sync_pb::PrinterSpecifics& local_printer = + local_data.specifics.printer(); + const sync_pb::PrinterSpecifics& remote_printer = + remote_data.specifics.printer(); + + if (local_printer.updated_timestamp() > remote_printer.updated_timestamp()) { + return ConflictResolution::UseLocal(); + } + + return ConflictResolution::UseRemote(); +} + +void PrintersSyncBridge::AddPrinter( + std::unique_ptr printer) { + printer->set_updated_timestamp(base::Time::Now().ToJavaTime()); + + std::unique_ptr batch = + store_delegate_->CreateWriteBatch(); + if (change_processor()->IsTrackingMetadata()) { + change_processor()->Put(printer->id(), CopyToEntityData(*printer), + batch->GetMetadataChangeList()); + } + + StoreSpecifics(std::move(printer), &all_data_, batch.get()); + store_delegate_->Commit(std::move(batch)); +} + +bool PrintersSyncBridge::RemovePrinter(const std::string& id) { + DCHECK(store_delegate_->Ready()); + + // Remove from local cache. + if (all_data_.erase(id) == 0) { + LOG(WARNING) << "Could not find printer" << id; + return false; + } + + std::unique_ptr batch = + store_delegate_->CreateWriteBatch(); + if (change_processor()->IsTrackingMetadata()) { + change_processor()->Delete(id, batch->GetMetadataChangeList()); + } + + // Remove from sync'd data. + batch->DeleteData(id); + store_delegate_->Commit(std::move(batch)); + + return true; +} + +std::vector PrintersSyncBridge::GetAllPrinters() + const { + std::vector printers; + for (auto& entry : all_data_) { + printers.push_back(*entry.second); + } + + return printers; +} + +base::Optional PrintersSyncBridge::GetPrinter( + const std::string& id) const { + auto iter = all_data_.find(id); + if (iter == all_data_.end()) { + return {}; + } + + return {*iter->second}; +} + +} // namespace chromeos diff --git a/chrome/browser/chromeos/printing/printers_sync_bridge.h b/chrome/browser/chromeos/printing/printers_sync_bridge.h new file mode 100644 index 00000000000000..971b86c0ca07f7 --- /dev/null +++ b/chrome/browser/chromeos/printing/printers_sync_bridge.h @@ -0,0 +1,73 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_CHROMEOS_PRINTING_PRINTERS_SYNC_BRIDGE_H_ +#define CHROME_BROWSER_CHROMEOS_PRINTING_PRINTERS_SYNC_BRIDGE_H_ + +#include +#include +#include +#include + +#include "base/optional.h" +#include "base/sequenced_task_runner.h" +#include "components/sync/model/conflict_resolution.h" +#include "components/sync/model/model_type_store.h" +#include "components/sync/model/model_type_sync_bridge.h" + +namespace sync_pb { +class PrinterSpecifics; +} + +namespace chromeos { + +// Moderates interaction with the backing database and integrates with the User +// Sync Service for printers. +class PrintersSyncBridge : public syncer::ModelTypeSyncBridge { + public: + PrintersSyncBridge(const syncer::ModelTypeStoreFactory& callback, + const base::RepeatingClosure& error_callback); + ~PrintersSyncBridge() override; + + // ModelTypeSyncBridge implementation. + std::unique_ptr CreateMetadataChangeList() + override; + base::Optional MergeSyncData( + std::unique_ptr metadata_change_list, + syncer::EntityDataMap entity_data_map) override; + base::Optional ApplySyncChanges( + std::unique_ptr metadata_change_list, + syncer::EntityChangeList entity_changes) override; + void GetData(StorageKeyList storage_keys, DataCallback callback) override; + void GetAllData(DataCallback callback) override; + std::string GetClientTag(const syncer::EntityData& entity_data) override; + std::string GetStorageKey(const syncer::EntityData& entity_data) override; + syncer::ConflictResolution ResolveConflict( + const syncer::EntityData& local_data, + const syncer::EntityData& remote_data) const override; + + // Store a new or updated |printer|. + void AddPrinter(std::unique_ptr printer); + // Remove a printer by |id|. + bool RemovePrinter(const std::string& id); + // Returns all printers stored in the database and synced. + std::vector GetAllPrinters() const; + // Returns the printer with |id| from storage if it could be found. + base::Optional GetPrinter( + const std::string& id) const; + + private: + class StoreProxy; + + std::unique_ptr store_delegate_; + + // In memory cache of printer information. + std::map> all_data_; + + DISALLOW_COPY_AND_ASSIGN(PrintersSyncBridge); +}; + +} // namespace chromeos + +#endif // CHROME_BROWSER_CHROMEOS_PRINTING_PRINTERS_SYNC_BRIDGE_H_ diff --git a/chrome/browser/chromeos/printing/specifics_translation.cc b/chrome/browser/chromeos/printing/specifics_translation.cc index 78503a1f2e0156..2a59b419f0526b 100644 --- a/chrome/browser/chromeos/printing/specifics_translation.cc +++ b/chrome/browser/chromeos/printing/specifics_translation.cc @@ -31,18 +31,15 @@ Printer::PpdReference SpecificsToPpd( return ref; } -sync_pb::PrinterPPDReference ReferenceToSpecifics( - const Printer::PpdReference& ref) { - sync_pb::PrinterPPDReference specifics; +void MergeReferenceToSpecifics(sync_pb::PrinterPPDReference* specifics, + const Printer::PpdReference& ref) { if (!ref.user_supplied_ppd_url.empty()) { - specifics.set_user_supplied_ppd_url(ref.user_supplied_ppd_url); + specifics->set_user_supplied_ppd_url(ref.user_supplied_ppd_url); } if (!ref.effective_make_and_model.empty()) { - specifics.set_effective_make_and_model(ref.effective_make_and_model); + specifics->set_effective_make_and_model(ref.effective_make_and_model); } - - return specifics; } } // namespace @@ -70,6 +67,14 @@ std::unique_ptr PrinterToSpecifics( auto specifics = base::MakeUnique(); specifics->set_id(printer.id()); + MergePrinterToSpecifics(printer, specifics.get()); + return specifics; +} + +void MergePrinterToSpecifics(const Printer& printer, + sync_pb::PrinterSpecifics* specifics) { + // Never update id it needs to be stable. + DCHECK_EQ(printer.id(), specifics->id()); if (!printer.display_name().empty()) specifics->set_display_name(printer.display_name()); @@ -89,10 +94,8 @@ std::unique_ptr PrinterToSpecifics( if (!printer.uuid().empty()) specifics->set_uuid(printer.uuid()); - *specifics->mutable_ppd_reference() = - ReferenceToSpecifics(printer.ppd_reference()); - - return specifics; + MergeReferenceToSpecifics(specifics->mutable_ppd_reference(), + printer.ppd_reference()); } } // namespace printing diff --git a/chrome/browser/chromeos/printing/specifics_translation.h b/chrome/browser/chromeos/printing/specifics_translation.h index fa844918da028b..382f766c3c7fec 100644 --- a/chrome/browser/chromeos/printing/specifics_translation.h +++ b/chrome/browser/chromeos/printing/specifics_translation.h @@ -19,6 +19,10 @@ std::unique_ptr SpecificsToPrinter( std::unique_ptr PrinterToSpecifics( const Printer& printer); +// Merge fields from |printer| into |specifics|. +void MergePrinterToSpecifics(const Printer& printer, + sync_pb::PrinterSpecifics* specifics); + } // namespace printing } // namespace chromeos diff --git a/chrome/browser/sync/chrome_sync_client.cc b/chrome/browser/sync/chrome_sync_client.cc index 09c30eedcd0ad7..ea1c17246b9477 100644 --- a/chrome/browser/sync/chrome_sync_client.cc +++ b/chrome/browser/sync/chrome_sync_client.cc @@ -104,6 +104,9 @@ #endif #if defined(OS_CHROMEOS) +#include "chrome/browser/chromeos/printing/printer_pref_manager.h" +#include "chrome/browser/chromeos/printing/printer_pref_manager_factory.h" +#include "chrome/browser/chromeos/printing/printers_sync_bridge.h" #include "chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h" #include "chrome/browser/ui/app_list/arc/arc_package_syncable_service.h" #include "components/sync_wifi/wifi_credential_syncable_service.h" @@ -448,6 +451,12 @@ ChromeSyncClient::GetSyncBridgeForModelType(syncer::ModelType type) { return autofill::AutocompleteSyncBridge::FromWebDataService( web_data_service_.get()) ->AsWeakPtr(); +#if defined(OS_CHROMEOS) + case syncer::PRINTERS: + return chromeos::PrinterPrefManagerFactory::GetForBrowserContext(profile_) + ->GetSyncBridge() + ->AsWeakPtr(); +#endif default: NOTREACHED(); return base::WeakPtr(); @@ -626,6 +635,7 @@ void ChromeSyncClient::RegisterDesktopDataTypes( syncer::WIFI_CREDENTIALS, error_callback, this, syncer::GROUP_UI, BrowserThread::GetTaskRunnerForThread(BrowserThread::UI))); } + // TODO(lgcheng): Add switch for this. sync_service->RegisterDataTypeController( base::MakeUnique( diff --git a/chrome/browser/sync/profile_sync_service_factory_unittest.cc b/chrome/browser/sync/profile_sync_service_factory_unittest.cc index b8315432b3524e..e326da8fc124bf 100644 --- a/chrome/browser/sync/profile_sync_service_factory_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_factory_unittest.cc @@ -41,9 +41,6 @@ class ProfileSyncServiceFactoryTest : public testing::Test { datatypes.push_back(syncer::APP_SETTINGS); #if defined(OS_LINUX) || defined(OS_WIN) || defined(OS_CHROMEOS) datatypes.push_back(syncer::DICTIONARY); -#endif -#if defined(OS_CHROMEOS) - datatypes.push_back(syncer::ARC_PACKAGE); #endif datatypes.push_back(syncer::EXTENSIONS); datatypes.push_back(syncer::EXTENSION_SETTINGS); @@ -53,6 +50,11 @@ class ProfileSyncServiceFactoryTest : public testing::Test { datatypes.push_back(syncer::SUPERVISED_USER_SHARED_SETTINGS); #endif // !OS_ANDROID +#if defined(OS_CHROMEOS) + datatypes.push_back(syncer::ARC_PACKAGE); + datatypes.push_back(syncer::PRINTERS); +#endif // OS_CHROMEOS + // Common types. datatypes.push_back(syncer::AUTOFILL); datatypes.push_back(syncer::AUTOFILL_PROFILE); diff --git a/chrome/browser/sync/test/integration/printers_helper.cc b/chrome/browser/sync/test/integration/printers_helper.cc new file mode 100644 index 00000000000000..2c0e3cc9634b76 --- /dev/null +++ b/chrome/browser/sync/test/integration/printers_helper.cc @@ -0,0 +1,161 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/sync/test/integration/printers_helper.h" + +#include +#include +#include +#include +#include + +#include "base/memory/ptr_util.h" +#include "base/run_loop.h" +#include "base/strings/stringprintf.h" +#include "chrome/browser/chromeos/printing/printer_pref_manager.h" +#include "chrome/browser/chromeos/printing/printer_pref_manager_factory.h" +#include "chrome/browser/sync/test/integration/sync_datatype_helper.h" +#include "chrome/browser/sync/test/integration/sync_test.h" + +using sync_datatype_helper::test; + +namespace printers_helper { + +namespace { + +using PrinterList = std::vector>; + +// Returns true if Printer#id, Printer#description, and Printer#uri all match. +bool PrintersAreMostlyEqual(const chromeos::Printer& left, + const chromeos::Printer& right) { + return left.id() == right.id() && left.description() == right.description() && + left.uri() == right.uri(); +} + +// Returns true if both lists have the same elements irrespective of order. +bool ListsContainTheSamePrinters(const PrinterList& list_a, + const PrinterList& list_b) { + std::unordered_multimap map_b; + for (const auto& b : list_b) { + map_b.insert({b->id(), b.get()}); + } + + for (const auto& a : list_a) { + auto range = map_b.equal_range(a->id()); + + auto it = std::find_if( + range.first, range.second, + [&a](const std::pair& entry) + -> bool { return PrintersAreMostlyEqual(*a, *(entry.second)); }); + + if (it == range.second) { + // Element in a does not match an element in b. Lists do not contain the + // same elements. + return false; + } + + map_b.erase(it); + } + + return map_b.empty(); +} + +std::string PrinterId(int index) { + return base::StringPrintf("printer%d", index); +} + +} // namespace + +void AddPrinter(chromeos::PrinterPrefManager* manager, + const chromeos::Printer& printer) { + manager->RegisterPrinter(base::MakeUnique(printer)); +} + +void RemovePrinter(chromeos::PrinterPrefManager* manager, int index) { + chromeos::Printer testPrinter(CreateTestPrinter(index)); + manager->RemovePrinter(testPrinter.id()); +} + +bool EditPrinterDescription(chromeos::PrinterPrefManager* manager, + int index, + const std::string& description) { + PrinterList printers = manager->GetPrinters(); + std::string printer_id = PrinterId(index); + auto found = std::find_if( + printers.begin(), printers.end(), + [&printer_id](const std::unique_ptr& printer) -> bool { + return printer->id() == printer_id; + }); + + if (found == printers.end()) + return false; + + (*found)->set_description(description); + manager->RegisterPrinter(std::move(*found)); + + return true; +} + +chromeos::Printer CreateTestPrinter(int index) { + chromeos::Printer printer(PrinterId(index)); + printer.set_description("Description"); + printer.set_uri(base::StringPrintf("ipp://192.168.1.%d", index)); + + return printer; +} + +chromeos::PrinterPrefManager* GetVerifierPrinterStore() { + chromeos::PrinterPrefManager* manager = + chromeos::PrinterPrefManagerFactory::GetForBrowserContext( + sync_datatype_helper::test()->verifier()); + // Must wait for ModelTypeStore initialization. + base::RunLoop().RunUntilIdle(); + + return manager; +} + +chromeos::PrinterPrefManager* GetPrinterStore(int index) { + chromeos::PrinterPrefManager* manager = + chromeos::PrinterPrefManagerFactory::GetForBrowserContext( + sync_datatype_helper::test()->GetProfile(index)); + // Must wait for ModelTypeStore initialization. + base::RunLoop().RunUntilIdle(); + + return manager; +} + +int GetVerifierPrinterCount() { + return GetVerifierPrinterStore()->GetPrinters().size(); +} + +int GetPrinterCount(int index) { + return GetPrinterStore(index)->GetPrinters().size(); +} + +bool AllProfilesContainSamePrinters() { + auto reference_printers = GetPrinterStore(0)->GetPrinters(); + for (int i = 1; i < test()->num_clients(); ++i) { + auto printers = GetPrinterStore(i)->GetPrinters(); + if (!ListsContainTheSamePrinters(reference_printers, printers)) { + VLOG(1) << "Printers in client [" << i << "] don't match client 0"; + return false; + } + } + + return true; +} + +bool ProfileContainsSamePrintersAsVerifier(int index) { + return ListsContainTheSamePrinters(GetVerifierPrinterStore()->GetPrinters(), + GetPrinterStore(index)->GetPrinters()); +} + +PrintersMatchChecker::PrintersMatchChecker() + : AwaitMatchStatusChangeChecker( + base::Bind(&printers_helper::AllProfilesContainSamePrinters), + "All printers match") {} + +PrintersMatchChecker::~PrintersMatchChecker() {} + +} // namespace printers_helper diff --git a/chrome/browser/sync/test/integration/printers_helper.h b/chrome/browser/sync/test/integration/printers_helper.h new file mode 100644 index 00000000000000..3a676dca33ac9b --- /dev/null +++ b/chrome/browser/sync/test/integration/printers_helper.h @@ -0,0 +1,75 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_SYNC_TEST_INTEGRATION_PRINTERS_HELPER_H_ +#define CHROME_BROWSER_SYNC_TEST_INTEGRATION_PRINTERS_HELPER_H_ + +#include +#include + +#include "chrome/browser/chromeos/printing/printer_pref_manager.h" +#include "chrome/browser/sync/test/integration/await_match_status_change_checker.h" +#include "chromeos/printing/printer_configuration.h" + +namespace printers_helper { + +void SetUp(); + +void SetupClients(); + +// Create a test printer. +chromeos::Printer CreateTestPrinter(int index); + +// Add printer to the supplied store. +void AddPrinter(chromeos::PrinterPrefManager* manager, + const chromeos::Printer& printer); + +// Remove printer |index| from the |manager|. +void RemovePrinter(chromeos::PrinterPrefManager* manager, int index); + +// Change the description of the printer at |index| with |description|. Returns +// false if the printer is not tracked by the manager. +bool EditPrinterDescription(chromeos::PrinterPrefManager* manager, + int index, + const std::string& description); + +// Returns the verifier store. +chromeos::PrinterPrefManager* GetVerifierPrinterStore(); + +// Returns printer store at |index|. +chromeos::PrinterPrefManager* GetPrinterStore(int index); + +// Returns the number of printers in the verifier store. +int GetVerifierPrinterCount(); + +// Returns the number of printers in printer store |index|. +int GetPrinterCount(int index); + +// Returns true if all profiles contain the same printers as profile 0. +bool AllProfilesContainSamePrinters(); + +// Returns true if the verifier store and printer store |index| contain the same +// data. +bool ProfileContainsSamePrintersAsVerifier(int index); + +// A waiter that can block until we can satisfy +// AllProfilesContainSamePrinters(). +// Example: +// // Make changes that you expect to sync. +// +// ASSERT_TRUE(PrintersMatchChecker().Wait()); +// +// // Sync is complete, verify the chagnes. +class PrintersMatchChecker : public AwaitMatchStatusChangeChecker { + public: + PrintersMatchChecker(); + ~PrintersMatchChecker() override; + + private: + DISALLOW_COPY_AND_ASSIGN(PrintersMatchChecker); +}; + +} // namespace printers_helper + +#endif // CHROME_BROWSER_SYNC_TEST_INTEGRATION_PRINTERS_HELPER_H_ diff --git a/chrome/browser/sync/test/integration/single_client_printers_sync_test.cc b/chrome/browser/sync/test/integration/single_client_printers_sync_test.cc new file mode 100644 index 00000000000000..4b41c7e3da990f --- /dev/null +++ b/chrome/browser/sync/test/integration/single_client_printers_sync_test.cc @@ -0,0 +1,89 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include "base/macros.h" +#include "chrome/browser/sync/test/integration/printers_helper.h" +#include "chrome/browser/sync/test/integration/sync_test.h" +#include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h" +#include "chromeos/printing/printer_configuration.h" + +using printers_helper::AddPrinter; +using printers_helper::CreateTestPrinter; +using printers_helper::EditPrinterDescription; +using printers_helper::GetVerifierPrinterCount; +using printers_helper::GetVerifierPrinterStore; +using printers_helper::GetPrinterCount; +using printers_helper::GetPrinterStore; +using printers_helper::ProfileContainsSamePrintersAsVerifier; +using printers_helper::RemovePrinter; + +class SingleClientPrintersSyncTest : public SyncTest { + public: + SingleClientPrintersSyncTest() : SyncTest(SINGLE_CLIENT) {} + ~SingleClientPrintersSyncTest() override {} + + private: + DISALLOW_COPY_AND_ASSIGN(SingleClientPrintersSyncTest); +}; + +// Verify that printers aren't added with a sync call. +IN_PROC_BROWSER_TEST_F(SingleClientPrintersSyncTest, NoPrinters) { + ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; + ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait()); + EXPECT_TRUE(ProfileContainsSamePrintersAsVerifier(0)); +} + +// Verify syncing doesn't randomly remove a printer. +IN_PROC_BROWSER_TEST_F(SingleClientPrintersSyncTest, SingleNewPrinter) { + ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; + + ASSERT_EQ(0, GetVerifierPrinterCount()); + + AddPrinter(GetPrinterStore(0), printers_helper::CreateTestPrinter(0)); + AddPrinter(GetVerifierPrinterStore(), printers_helper::CreateTestPrinter(0)); + ASSERT_EQ(1, GetPrinterCount(0)); + ASSERT_EQ(1, GetVerifierPrinterCount()); + + ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait()); + EXPECT_EQ(1, GetVerifierPrinterCount()); + EXPECT_TRUE(ProfileContainsSamePrintersAsVerifier(0)); +} + +// Verify editing a printer doesn't add it. +IN_PROC_BROWSER_TEST_F(SingleClientPrintersSyncTest, EditPrinter) { + ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; + + AddPrinter(GetPrinterStore(0), printers_helper::CreateTestPrinter(0)); + AddPrinter(GetVerifierPrinterStore(), printers_helper::CreateTestPrinter(0)); + + ASSERT_TRUE( + EditPrinterDescription(GetPrinterStore(0), 0, "Updated description")); + + EXPECT_EQ(1, GetPrinterCount(0)); + EXPECT_EQ(1, GetVerifierPrinterCount()); + EXPECT_FALSE(ProfileContainsSamePrintersAsVerifier(0)); +} + +// Verify that removing a printer works. +IN_PROC_BROWSER_TEST_F(SingleClientPrintersSyncTest, RemovePrinter) { + ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; + + AddPrinter(GetPrinterStore(0), printers_helper::CreateTestPrinter(0)); + EXPECT_EQ(1, GetPrinterCount(0)); + + RemovePrinter(GetPrinterStore(0), 0); + EXPECT_EQ(0, GetPrinterCount(0)); +} + +// Verify that merging data added before sync works. +IN_PROC_BROWSER_TEST_F(SingleClientPrintersSyncTest, AddBeforeSetup) { + ASSERT_TRUE(SetupClients()); + + AddPrinter(GetPrinterStore(0), printers_helper::CreateTestPrinter(0)); + EXPECT_EQ(1, GetPrinterCount(0)); + + EXPECT_TRUE(SetupSync()) << "SetupSync() failed."; +} diff --git a/chrome/browser/sync/test/integration/two_client_printers_sync_test.cc b/chrome/browser/sync/test/integration/two_client_printers_sync_test.cc new file mode 100644 index 00000000000000..96338833df49c5 --- /dev/null +++ b/chrome/browser/sync/test/integration/two_client_printers_sync_test.cc @@ -0,0 +1,150 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include "base/macros.h" +#include "base/run_loop.h" +#include "chrome/browser/sync/test/integration/printers_helper.h" +#include "chrome/browser/sync/test/integration/sync_test.h" + +namespace { + +using printers_helper::AddPrinter; +using printers_helper::AllProfilesContainSamePrinters; +using printers_helper::EditPrinterDescription; +using printers_helper::CreateTestPrinter; +using printers_helper::GetPrinterCount; +using printers_helper::GetPrinterStore; +using printers_helper::PrintersMatchChecker; +using printers_helper::RemovePrinter; + +class TwoClientPrintersSyncTest : public SyncTest { + public: + TwoClientPrintersSyncTest() : SyncTest(TWO_CLIENT) {} + ~TwoClientPrintersSyncTest() override {} + + private: + DISALLOW_COPY_AND_ASSIGN(TwoClientPrintersSyncTest); +}; + +IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, NoPrinters) { + ASSERT_TRUE(SetupSync()); + + ASSERT_TRUE(PrintersMatchChecker().Wait()); +} + +IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, OnePrinter) { + ASSERT_TRUE(SetupSync()); + + AddPrinter(GetPrinterStore(0), CreateTestPrinter(2)); + + ASSERT_TRUE(PrintersMatchChecker().Wait()); +} + +IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, SimultaneousAdd) { + ASSERT_TRUE(SetupSync()); + + AddPrinter(GetPrinterStore(0), CreateTestPrinter(1)); + AddPrinter(GetPrinterStore(1), CreateTestPrinter(2)); + + // Each store is guaranteed to have 1 printer because the tests run on the UI + // thread. ApplySyncChanges happens after we wait on the checker. + ASSERT_EQ(1, GetPrinterCount(0)); + ASSERT_EQ(1, GetPrinterCount(1)); + + ASSERT_TRUE(PrintersMatchChecker().Wait()); + EXPECT_EQ(2, GetPrinterCount(0)); +} + +IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, RemovePrinter) { + ASSERT_TRUE(SetupSync()); + + AddPrinter(GetPrinterStore(0), CreateTestPrinter(1)); + AddPrinter(GetPrinterStore(0), CreateTestPrinter(2)); + AddPrinter(GetPrinterStore(0), CreateTestPrinter(3)); + + // Verify the profiles have the same printers + ASSERT_TRUE(PrintersMatchChecker().Wait()); + EXPECT_EQ(3, GetPrinterCount(0)); + + // Remove printer 2 from store 1 + RemovePrinter(GetPrinterStore(1), 2); + + ASSERT_TRUE(PrintersMatchChecker().Wait()); + EXPECT_EQ(2, GetPrinterCount(0)); +} + +IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, RemoveAndEditPrinters) { + const std::string updated_description = "Testing changes"; + + ASSERT_TRUE(SetupSync()); + + AddPrinter(GetPrinterStore(0), CreateTestPrinter(1)); + AddPrinter(GetPrinterStore(0), CreateTestPrinter(2)); + AddPrinter(GetPrinterStore(0), CreateTestPrinter(3)); + + // Verify the profiles have the same printers + ASSERT_TRUE(PrintersMatchChecker().Wait()); + EXPECT_EQ(3, GetPrinterCount(0)); + + // Edit printer 1 from store 0 + ASSERT_TRUE( + EditPrinterDescription(GetPrinterStore(0), 1, updated_description)); + + // Remove printer 2 from store 1 + RemovePrinter(GetPrinterStore(1), 2); + + ASSERT_TRUE(PrintersMatchChecker().Wait()); + EXPECT_EQ(2, GetPrinterCount(0)); + EXPECT_EQ(updated_description, + GetPrinterStore(1)->GetPrinters()[0]->description()); +} + +IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, ConflictResolution) { + ASSERT_TRUE(SetupSync()); + + AddPrinter(GetPrinterStore(0), CreateTestPrinter(0)); + AddPrinter(GetPrinterStore(0), CreateTestPrinter(2)); + AddPrinter(GetPrinterStore(0), CreateTestPrinter(3)); + + // Store 0 and 1 have 3 printers now. + ASSERT_TRUE(PrintersMatchChecker().Wait()); + + std::string overwritten_message = "I should not show up"; + ASSERT_TRUE( + EditPrinterDescription(GetPrinterStore(1), 0, overwritten_message)); + + // Wait for a non-zero period (200ms). + base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(200)); + + std::string valid_message = "YAY! More recent changes win!"; + ASSERT_TRUE(EditPrinterDescription(GetPrinterStore(0), 0, valid_message)); + + // Conflict resolution shoud run here. + ASSERT_TRUE(PrintersMatchChecker().Wait()); + // The more recent update should win. + EXPECT_EQ(valid_message, GetPrinterStore(1)->GetPrinters()[0]->description()); +} + +IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, SimpleMerge) { + ASSERT_TRUE(SetupClients()); + base::RunLoop().RunUntilIdle(); + + // Store 0 has the even printers + AddPrinter(GetPrinterStore(0), CreateTestPrinter(0)); + AddPrinter(GetPrinterStore(0), CreateTestPrinter(2)); + + // Store 1 has the odd printers + AddPrinter(GetPrinterStore(1), CreateTestPrinter(1)); + AddPrinter(GetPrinterStore(1), CreateTestPrinter(3)); + + ASSERT_TRUE(SetupSync()); + + // Stores should contain the same values now. + EXPECT_EQ(4, GetPrinterCount(0)); + EXPECT_TRUE(AllProfilesContainSamePrinters()); +} + +} // namespace diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 7f97b8e5d2edfb..26ba839efa3c85 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -849,6 +849,8 @@ if (!is_android) { "../browser/sync/test/integration/passwords_helper.h", "../browser/sync/test/integration/preferences_helper.cc", "../browser/sync/test/integration/preferences_helper.h", + "../browser/sync/test/integration/printers_helper.cc", + "../browser/sync/test/integration/printers_helper.h", "../browser/sync/test/integration/profile_sync_service_harness.cc", "../browser/sync/test/integration/profile_sync_service_harness.h", "../browser/sync/test/integration/quiesce_status_change_checker.cc", @@ -925,6 +927,8 @@ if (!is_android) { } if (!is_chromeos) { sources -= [ + "../browser/sync/test/integration/printers_helper.cc", + "../browser/sync/test/integration/printers_helper.h", "../browser/sync/test/integration/sync_arc_package_helper.cc", "../browser/sync/test/integration/sync_arc_package_helper.h", "../browser/sync/test/integration/wifi_credentials_helper.cc", @@ -2719,6 +2723,7 @@ if (!is_android) { "../browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc", "../browser/sync/test/integration/single_client_passwords_sync_test.cc", "../browser/sync/test/integration/single_client_preferences_sync_test.cc", + "../browser/sync/test/integration/single_client_printers_sync_test.cc", "../browser/sync/test/integration/single_client_search_engines_sync_test.cc", "../browser/sync/test/integration/single_client_sessions_sync_test.cc", "../browser/sync/test/integration/single_client_supervised_user_settings_sync_test.cc", @@ -2740,6 +2745,7 @@ if (!is_android) { "../browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc", "../browser/sync/test/integration/two_client_passwords_sync_test.cc", "../browser/sync/test/integration/two_client_preferences_sync_test.cc", + "../browser/sync/test/integration/two_client_printers_sync_test.cc", "../browser/sync/test/integration/two_client_search_engines_sync_test.cc", "../browser/sync/test/integration/two_client_sessions_sync_test.cc", "../browser/sync/test/integration/two_client_themes_sync_test.cc", @@ -2824,8 +2830,10 @@ if (!is_android) { if (!is_chromeos) { sources -= [ "../browser/sync/test/integration/single_client_arc_package_sync_test.cc", + "../browser/sync/test/integration/single_client_printers_sync_test.cc", "../browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc", "../browser/sync/test/integration/two_client_arc_package_sync_test.cc", + "../browser/sync/test/integration/two_client_printers_sync_test.cc", "../browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc", ] } diff --git a/chromeos/printing/printer_configuration.h b/chromeos/printing/printer_configuration.h index d034384225f070..27f047bdb93e39 100644 --- a/chromeos/printing/printer_configuration.h +++ b/chromeos/printing/printer_configuration.h @@ -50,7 +50,7 @@ class CHROMEOS_EXPORT Printer { explicit Printer(const std::string& id); // Copy constructor and assignment. - explicit Printer(const Printer& printer); + Printer(const Printer& printer); Printer& operator=(const Printer& printer); ~Printer(); diff --git a/components/browser_sync/profile_sync_components_factory_impl.cc b/components/browser_sync/profile_sync_components_factory_impl.cc index ba1f82b5675231..2d933196b3b9b5 100644 --- a/components/browser_sync/profile_sync_components_factory_impl.cc +++ b/components/browser_sync/profile_sync_components_factory_impl.cc @@ -283,6 +283,14 @@ void ProfileSyncComponentsFactoryImpl::RegisterCommonDataTypes( ui_thread_)); } +#if defined(OS_CHROMEOS) + if (!disabled_types.Has(syncer::PRINTERS)) { + sync_service->RegisterDataTypeController( + base::MakeUnique(syncer::PRINTERS, sync_client_, + ui_thread_)); + } +#endif + // Reading list sync is enabled by default only on iOS. Register unless // Reading List or Reading List Sync is explicitly disabled. if (!disabled_types.Has(syncer::READING_LIST) &&