Remove Device::Options struct BUG:24267885 Change-Id: I4d4f18f3c4658495a2521c79d87f049941020320 Reviewed-on: https://weave-review.googlesource.com/1195 Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/libweave/examples/ubuntu/file_config_store.cc b/libweave/examples/ubuntu/file_config_store.cc index 49fa907..d340884 100644 --- a/libweave/examples/ubuntu/file_config_store.cc +++ b/libweave/examples/ubuntu/file_config_store.cc
@@ -18,6 +18,9 @@ const char kSettingsDir[] = "/var/lib/weave/"; const char kSettingsPath[] = "/var/lib/weave/weave_settings.json"; +FileConfigStore::FileConfigStore(bool disable_security) + : disable_security_{disable_security} {} + bool FileConfigStore::LoadDefaults(Settings* settings) { char host_name[HOST_NAME_MAX] = {}; gethostname(host_name, HOST_NAME_MAX); @@ -37,6 +40,8 @@ settings->client_id = "58855907228.apps.googleusercontent.com"; settings->client_secret = "eHSAREAHrIqPsHBxCE9zPPBi"; settings->api_key = "AIzaSyDSq46gG-AxUnC3zoqD9COIPrjolFsMfMA"; + + settings->disable_security = disable_security_; return true; }
diff --git a/libweave/examples/ubuntu/file_config_store.h b/libweave/examples/ubuntu/file_config_store.h index e2bd1c1..2014f86 100644 --- a/libweave/examples/ubuntu/file_config_store.h +++ b/libweave/examples/ubuntu/file_config_store.h
@@ -16,6 +16,8 @@ class FileConfigStore : public provider::ConfigStore { public: + explicit FileConfigStore(bool disable_security); + bool LoadDefaults(Settings* settings) override; std::string LoadSettings() override; void SaveSettings(const std::string& settings) override; @@ -26,6 +28,9 @@ std::string LoadBaseStateDefaults() override; std::map<std::string, std::string> LoadStateDefs() override; std::vector<std::string> LoadStateDefaults() override; + + private: + bool disable_security_{false}; }; } // namespace examples
diff --git a/libweave/examples/ubuntu/main.cc b/libweave/examples/ubuntu/main.cc index 0bb423d..85fbe3b 100644 --- a/libweave/examples/ubuntu/main.cc +++ b/libweave/examples/ubuntu/main.cc
@@ -84,7 +84,7 @@ } } - weave::examples::FileConfigStore config_store; + weave::examples::FileConfigStore config_store{disable_security}; weave::examples::EventTaskRunner task_runner; weave::examples::CurlHttpClient http_client{&task_runner}; weave::examples::NetworkImpl network{&task_runner, force_bootstrapping}; @@ -93,13 +93,8 @@ weave::examples::BluetoothImpl bluetooth; auto device = weave::Device::Create(); - weave::Device::Options opts; - opts.xmpp_enabled = true; - opts.disable_privet = false; - opts.disable_security = disable_security; - opts.enable_ping = true; device->Start( - opts, &config_store, &task_runner, &http_client, &network, &dns_sd, + &config_store, &task_runner, &http_client, &network, &dns_sd, &http_server, weave::examples::NetworkImpl::HasWifiCapability() ? &network : nullptr, &bluetooth);
diff --git a/libweave/include/weave/device.h b/libweave/include/weave/device.h index 6d1cd92..39e2f21 100644 --- a/libweave/include/weave/device.h +++ b/libweave/include/weave/device.h
@@ -27,18 +27,9 @@ class Device { public: - struct Options { - bool xmpp_enabled = true; - bool disable_privet = false; - bool disable_security = false; - bool enable_ping = false; - std::string test_privet_ssid; - }; - virtual ~Device() = default; - virtual void Start(const Options& options, - provider::ConfigStore* config_store, + virtual void Start(provider::ConfigStore* config_store, provider::TaskRunner* task_runner, provider::HttpClient* http_client, provider::Network* network,
diff --git a/libweave/include/weave/settings.h b/libweave/include/weave/settings.h index 1c919a1..2b38c91 100644 --- a/libweave/include/weave/settings.h +++ b/libweave/include/weave/settings.h
@@ -48,14 +48,14 @@ std::string oauth_url; std::string service_url; - // Cloud ID of the registered device. Empty of device is not registered. + // Cloud ID of the registered device. Empty if device is not registered. std::string cloud_id; - // Internal options used by libweave. External code should not use them. - base::TimeDelta polling_period; - base::TimeDelta backup_polling_period; + // Internal options to tweak some library functionality. External code should + // avoid using them. bool wifi_auto_setup_enabled{true}; - bool ble_setup_enabled{false}; + bool disable_security{false}; + std::string test_privet_ssid; }; } // namespace weave
diff --git a/libweave/src/base_api_handler_unittest.cc b/libweave/src/base_api_handler_unittest.cc index 44c4825..5e2f68e 100644 --- a/libweave/src/base_api_handler_unittest.cc +++ b/libweave/src/base_api_handler_unittest.cc
@@ -59,7 +59,7 @@ std::unique_ptr<Config> config{new Config{&config_store_}}; config->Load(); dev_reg_.reset(new DeviceRegistrationInfo(command_manager_, state_manager_, - true, std::move(config), nullptr, + std::move(config), nullptr, &http_client_, nullptr)); handler_.reset( new BaseApiHandler{dev_reg_.get(), state_manager_, command_manager_});
diff --git a/libweave/src/config.cc b/libweave/src/config.cc index f601b82..e01e486 100644 --- a/libweave/src/config.cc +++ b/libweave/src/config.cc
@@ -52,12 +52,6 @@ result.oauth_url = "https://accounts.google.com/o/oauth2/"; result.service_url = "https://www.googleapis.com/clouddevices/v1/"; result.local_anonymous_access_role = "viewer"; - result.local_discovery_enabled = true; - result.local_pairing_enabled = true; - result.polling_period = base::TimeDelta::FromSeconds(7); - result.backup_polling_period = base::TimeDelta::FromMinutes(30); - result.wifi_auto_setup_enabled = true; - result.ble_setup_enabled = false; result.pairing_modes.emplace(PairingType::kPinCode); return result; }
diff --git a/libweave/src/config_unittest.cc b/libweave/src/config_unittest.cc index 02885e1..2639c22 100644 --- a/libweave/src/config_unittest.cc +++ b/libweave/src/config_unittest.cc
@@ -31,7 +31,9 @@ const Config::Settings& GetSettings() const { return config_.GetSettings(); } - const Config::Settings& GetDefaultSettings() const { return default_.GetSettings(); } + const Config::Settings& GetDefaultSettings() const { + return default_.GetSettings(); + } MOCK_METHOD1(OnConfigChanged, void(const Settings&)); @@ -57,11 +59,9 @@ EXPECT_EQ("", GetSettings().model_name); EXPECT_EQ("", GetSettings().model_id); EXPECT_EQ("", GetSettings().firmware_version); - EXPECT_EQ(base::TimeDelta::FromSeconds(7), GetSettings().polling_period); - EXPECT_EQ(base::TimeDelta::FromMinutes(30), - GetSettings().backup_polling_period); EXPECT_TRUE(GetSettings().wifi_auto_setup_enabled); - EXPECT_FALSE(GetSettings().ble_setup_enabled); + EXPECT_FALSE(GetSettings().disable_security); + EXPECT_EQ("", GetSettings().test_privet_ssid); EXPECT_EQ(std::set<PairingType>{PairingType::kPinCode}, GetSettings().pairing_modes); EXPECT_EQ("", GetSettings().embedded_code); @@ -110,13 +110,12 @@ EXPECT_EQ(GetDefaultSettings().oem_name, GetSettings().oem_name); EXPECT_EQ(GetDefaultSettings().model_name, GetSettings().model_name); EXPECT_EQ(GetDefaultSettings().model_id, GetSettings().model_id); - EXPECT_EQ(GetDefaultSettings().polling_period, GetSettings().polling_period); - EXPECT_EQ(GetDefaultSettings().backup_polling_period, - GetSettings().backup_polling_period); EXPECT_EQ(GetDefaultSettings().wifi_auto_setup_enabled, GetSettings().wifi_auto_setup_enabled); - EXPECT_EQ(GetDefaultSettings().ble_setup_enabled, - GetSettings().ble_setup_enabled); + EXPECT_EQ(GetDefaultSettings().disable_security, + GetSettings().disable_security); + EXPECT_EQ(GetDefaultSettings().test_privet_ssid, + GetSettings().test_privet_ssid); EXPECT_EQ(GetDefaultSettings().pairing_modes, GetSettings().pairing_modes); EXPECT_EQ(GetDefaultSettings().embedded_code, GetSettings().embedded_code); EXPECT_EQ("state_name", GetSettings().name);
diff --git a/libweave/src/device_manager.cc b/libweave/src/device_manager.cc index 17c628b..e7ddb23 100644 --- a/libweave/src/device_manager.cc +++ b/libweave/src/device_manager.cc
@@ -29,8 +29,7 @@ DeviceManager::~DeviceManager() {} -void DeviceManager::Start(const Options& options, - provider::ConfigStore* config_store, +void DeviceManager::Start(provider::ConfigStore* config_store, provider::TaskRunner* task_runner, provider::HttpClient* http_client, provider::Network* network, @@ -50,18 +49,17 @@ // TODO(avakulenko): Figure out security implications of storing // device info state data unencrypted. device_info_.reset(new DeviceRegistrationInfo( - command_manager_, state_manager_, options.xmpp_enabled, std::move(config), - task_runner, http_client, network)); + command_manager_, state_manager_, std::move(config), task_runner, + http_client, network)); base_api_handler_.reset( new BaseApiHandler{device_info_.get(), state_manager_, command_manager_}); device_info_->Start(); - if (!options.disable_privet) { - StartPrivet(options, task_runner, network, dns_sd, http_server, wifi, - bluetooth); + if (http_server) { + CHECK(dns_sd); + StartPrivet(task_runner, network, dns_sd, http_server, wifi, bluetooth); } else { - CHECK(!http_server); CHECK(!dns_sd); } } @@ -86,15 +84,14 @@ return privet_.get(); } -void DeviceManager::StartPrivet(const Options& options, - provider::TaskRunner* task_runner, +void DeviceManager::StartPrivet(provider::TaskRunner* task_runner, provider::Network* network, provider::DnsServiceDiscovery* dns_sd, provider::HttpServer* http_server, provider::Wifi* wifi, provider::Bluetooth* bluetooth) { privet_.reset(new privet::Manager{}); - privet_->Start(options, task_runner, network, dns_sd, http_server, wifi, + privet_->Start(task_runner, network, dns_sd, http_server, wifi, device_info_.get(), command_manager_.get(), state_manager_.get()); }
diff --git a/libweave/src/device_manager.h b/libweave/src/device_manager.h index 41d538e..294a7cd 100644 --- a/libweave/src/device_manager.h +++ b/libweave/src/device_manager.h
@@ -26,8 +26,7 @@ DeviceManager(); ~DeviceManager() override; - void Start(const Options& options, - provider::ConfigStore* config_store, + void Start(provider::ConfigStore* config_store, provider::TaskRunner* task_runner, provider::HttpClient* http_client, provider::Network* network, @@ -44,8 +43,7 @@ Config* GetConfig(); private: - void StartPrivet(const Options& options, - provider::TaskRunner* task_runner, + void StartPrivet(provider::TaskRunner* task_runner, provider::Network* network, provider::DnsServiceDiscovery* dns_sd, provider::HttpServer* http_server,
diff --git a/libweave/src/device_registration_info.cc b/libweave/src/device_registration_info.cc index a5d2f11..f1f94d0 100644 --- a/libweave/src/device_registration_info.cc +++ b/libweave/src/device_registration_info.cc
@@ -40,6 +40,9 @@ namespace { +const int kPollingPeriodSeconds = 7; +const int kBackupPollingPeriodSeconds = 30; + using provider::HttpClient; inline void SetUnexpectedError(ErrorPtr* error) { @@ -216,7 +219,6 @@ DeviceRegistrationInfo::DeviceRegistrationInfo( const std::shared_ptr<CommandManager>& command_manager, const std::shared_ptr<StateManager>& state_manager, - bool notifications_enabled, std::unique_ptr<Config> config, provider::TaskRunner* task_runner, provider::HttpClient* http_client, @@ -226,7 +228,6 @@ command_manager_{command_manager}, state_manager_{state_manager}, config_{std::move(config)}, - notifications_enabled_{notifications_enabled}, network_{network} { cloud_backoff_policy_.reset(new BackoffEntry::Policy{}); cloud_backoff_policy_->num_errors_to_ignore = 0; @@ -454,7 +455,8 @@ // call back to OnConnected() and at that time we'll switch to use the // primary channel and switch periodic poll into much more infrequent backup // poll mode. - const base::TimeDelta pull_interval = GetSettings().polling_period; + const base::TimeDelta pull_interval = + base::TimeDelta::FromSeconds(kPollingPeriodSeconds); if (!pull_channel_) { pull_channel_.reset(new PullChannel{pull_interval, task_runner_}); pull_channel_->Start(this); @@ -463,11 +465,6 @@ } current_notification_channel_ = pull_channel_.get(); - if (!notifications_enabled_) { - LOG(WARNING) << "Notification channel disabled by flag."; - return; - } - notification_channel_starting_ = true; primary_notification_channel_.reset(new XmppChannel{ GetSettings().robot_account, access_token_, task_runner_, network_}); @@ -1231,7 +1228,8 @@ << channel_name; CHECK_EQ(primary_notification_channel_->GetName(), channel_name); notification_channel_starting_ = false; - pull_channel_->UpdatePullInterval(GetSettings().backup_polling_period); + pull_channel_->UpdatePullInterval( + base::TimeDelta::FromSeconds(kBackupPollingPeriodSeconds)); current_notification_channel_ = primary_notification_channel_.get(); // If we have not successfully connected to the cloud server and we have not @@ -1255,7 +1253,8 @@ if (!HaveRegistrationCredentials() || !connected_to_cloud_) return; - pull_channel_->UpdatePullInterval(GetSettings().polling_period); + pull_channel_->UpdatePullInterval( + base::TimeDelta::FromSeconds(kPollingPeriodSeconds)); current_notification_channel_ = pull_channel_.get(); UpdateDeviceResource(base::Bind(&base::DoNothing), base::Bind(&IgnoreCloudError));
diff --git a/libweave/src/device_registration_info.h b/libweave/src/device_registration_info.h index 14ddc1d..bd08ecd 100644 --- a/libweave/src/device_registration_info.h +++ b/libweave/src/device_registration_info.h
@@ -57,7 +57,6 @@ DeviceRegistrationInfo(const std::shared_ptr<CommandManager>& command_manager, const std::shared_ptr<StateManager>& state_manager, - bool notifications_enabled, std::unique_ptr<Config> config, provider::TaskRunner* task_runner, provider::HttpClient* http_client, @@ -330,7 +329,6 @@ // another request is in flight to the cloud server. ResourceUpdateCallbackList queued_resource_update_callbacks_; - const bool notifications_enabled_; std::unique_ptr<NotificationChannel> primary_notification_channel_; std::unique_ptr<PullChannel> pull_channel_; NotificationChannel* current_notification_channel_{nullptr};
diff --git a/libweave/src/device_registration_info_unittest.cc b/libweave/src/device_registration_info_unittest.cc index 10bbc07..1a84558 100644 --- a/libweave/src/device_registration_info_unittest.cc +++ b/libweave/src/device_registration_info_unittest.cc
@@ -123,7 +123,7 @@ std::unique_ptr<Config> config{new Config{&config_store_}}; config_ = config.get(); dev_reg_.reset(new DeviceRegistrationInfo{command_manager_, state_manager_, - true, std::move(config), nullptr, + std::move(config), nullptr, &http_client_, nullptr}); ReloadDefaults(); @@ -496,7 +496,8 @@ // Validate the device info saved to storage... EXPECT_EQ(test_data::kDeviceId, dev_reg_->GetSettings().cloud_id); EXPECT_EQ(test_data::kRefreshToken, dev_reg_->GetSettings().refresh_token); - EXPECT_EQ(test_data::kRobotAccountEmail, dev_reg_->GetSettings().robot_account); + EXPECT_EQ(test_data::kRobotAccountEmail, + dev_reg_->GetSettings().robot_account); } TEST_F(DeviceRegistrationInfoTest, OOBRegistrationStatus) {
diff --git a/libweave/src/privet/privet_manager.cc b/libweave/src/privet/privet_manager.cc index d0ebdd7..d14765d 100644 --- a/libweave/src/privet/privet_manager.cc +++ b/libweave/src/privet/privet_manager.cc
@@ -39,8 +39,7 @@ Manager::~Manager() {} -void Manager::Start(const Device::Options& options, - TaskRunner* task_runner, +void Manager::Start(TaskRunner* task_runner, Network* network, DnsServiceDiscovery* dns_sd, HttpServer* http_server, @@ -48,7 +47,7 @@ DeviceRegistrationInfo* device, CommandManager* command_manager, StateManager* state_manager) { - disable_security_ = options.disable_security; + disable_security_ = device->GetSettings().disable_security; device_ = DeviceDelegate::CreateDefault(); cloud_ = CloudDelegate::CreateDefault(task_runner, device, command_manager, @@ -71,8 +70,7 @@ if (wifi && device->GetSettings().wifi_auto_setup_enabled) { VLOG(1) << "Enabling WiFi bootstrapping."; wifi_bootstrap_manager_.reset(new WifiBootstrapManager( - options.test_privet_ssid, device->GetMutableConfig(), task_runner, - network, wifi, cloud_.get())); + device->GetMutableConfig(), task_runner, network, wifi, cloud_.get())); wifi_bootstrap_manager_->Init(); } @@ -88,11 +86,6 @@ http_server->AddRequestHandler("/privet/", base::Bind(&Manager::PrivetRequestHandler, weak_ptr_factory_.GetWeakPtr())); - if (options.enable_ping) { - http_server->AddRequestHandler("/privet/ping", - base::Bind(&Manager::HelloWorldHandler, - weak_ptr_factory_.GetWeakPtr())); - } } std::string Manager::GetCurrentlyConnectedSsid() const { @@ -148,11 +141,6 @@ callback.Run(status, data, http::kJson); } -void Manager::HelloWorldHandler(const HttpServer::Request& request, - const HttpServer::OnReplyCallback& callback) { - callback.Run(http::kOk, "Hello, world!", http::kPlain); -} - void Manager::OnChanged() { if (publisher_) publisher_->Update();
diff --git a/libweave/src/privet/privet_manager.h b/libweave/src/privet/privet_manager.h index 3daecbd..3881971 100644 --- a/libweave/src/privet/privet_manager.h +++ b/libweave/src/privet/privet_manager.h
@@ -47,8 +47,7 @@ Manager(); ~Manager() override; - void Start(const weave::Device::Options& options, - provider::TaskRunner* task_runner, + void Start(provider::TaskRunner* task_runner, provider::Network* network, provider::DnsServiceDiscovery* dns_sd, provider::HttpServer* http_server, @@ -76,9 +75,6 @@ int status, const base::DictionaryValue& output); - void HelloWorldHandler(const provider::HttpServer::Request& request, - const provider::HttpServer::OnReplyCallback& callback); - void OnChanged(); void OnConnectivityChanged();
diff --git a/libweave/src/privet/wifi_bootstrap_manager.cc b/libweave/src/privet/wifi_bootstrap_manager.cc index 7fc9db5..ee8b15e 100644 --- a/libweave/src/privet/wifi_bootstrap_manager.cc +++ b/libweave/src/privet/wifi_bootstrap_manager.cc
@@ -20,8 +20,7 @@ using provider::Network; -WifiBootstrapManager::WifiBootstrapManager(const std::string& test_privet_ssid, - Config* config, +WifiBootstrapManager::WifiBootstrapManager(Config* config, provider::TaskRunner* task_runner, provider::Network* network, provider::Wifi* wifi, @@ -30,8 +29,8 @@ task_runner_{task_runner}, network_{network}, wifi_{wifi}, - ssid_generator_{gcd, this}, - test_privet_ssid_{test_privet_ssid} { + ssid_generator_{gcd, this} { + CHECK(config_); CHECK(network_); CHECK(task_runner_); CHECK(wifi_); @@ -74,13 +73,9 @@ privet_ssid_ = GenerateSsid(); CHECK(!privet_ssid_.empty()); wifi_->StartAccessPoint(privet_ssid_); - LOG_IF(INFO, config_->GetSettings().ble_setup_enabled) - << "BLE Bootstrap start: not implemented."; } void WifiBootstrapManager::EndBootstrapping() { - LOG_IF(INFO, config_->GetSettings().ble_setup_enabled) - << "BLE Bootstrap stop: not implemented."; wifi_->StopAccessPoint(); privet_ssid_.clear(); } @@ -159,8 +154,8 @@ } std::string WifiBootstrapManager::GenerateSsid() const { - return test_privet_ssid_.empty() ? ssid_generator_.GenerateSsid() - : test_privet_ssid_; + const std::string& ssid = config_->GetSettings().test_privet_ssid; + return ssid.empty() ? ssid_generator_.GenerateSsid() : ssid; } const ConnectionState& WifiBootstrapManager::GetConnectionState() const {
diff --git a/libweave/src/privet/wifi_bootstrap_manager.h b/libweave/src/privet/wifi_bootstrap_manager.h index 52886f9..aa908a3 100644 --- a/libweave/src/privet/wifi_bootstrap_manager.h +++ b/libweave/src/privet/wifi_bootstrap_manager.h
@@ -39,8 +39,7 @@ public: using State = WifiSetupState; - WifiBootstrapManager(const std::string& test_privet_ssid, - Config* config, + WifiBootstrapManager(Config* config, provider::TaskRunner* task_runner, provider::Network* shill_client, provider::Wifi* wifi, @@ -103,7 +102,6 @@ base::Time monitor_until_; bool currently_online_{false}; - std::string test_privet_ssid_; std::string privet_ssid_; // Helps to reset irrelevant tasks switching state.
diff --git a/libweave/src/weave_unittest.cc b/libweave/src/weave_unittest.cc index d42a839..fab3fad 100644 --- a/libweave/src/weave_unittest.cc +++ b/libweave/src/weave_unittest.cc
@@ -278,11 +278,8 @@ } void StartDevice() { - weave::Device::Options options; - options.xmpp_enabled = false; - - device_->Start(options, &config_store_, &task_runner_, &http_client_, - &network_, &dns_sd_, &http_server_, &wifi_, &bluetooth_); + device_->Start(&config_store_, &task_runner_, &http_client_, &network_, + &dns_sd_, &http_server_, &wifi_, &bluetooth_); cloud_ = device_->GetCloud(); ASSERT_TRUE(cloud_); @@ -333,14 +330,9 @@ } TEST_F(WeaveTest, StartMinimal) { - weave::Device::Options options; - options.xmpp_enabled = false; - options.disable_privet = true; - options.disable_security = true; - InitConfigStore(); - device_->Start(options, &config_store_, &task_runner_, &http_client_, - &network_, nullptr, nullptr, &wifi_, nullptr); + device_->Start(&config_store_, &task_runner_, &http_client_, &network_, + nullptr, nullptr, &wifi_, nullptr); } TEST_F(WeaveTest, StartNoWifi) { @@ -350,9 +342,8 @@ InitDnsSd(); InitDnsSdPublishing(false, "CB"); - weave::Device::Options options; - device_->Start(options, &config_store_, &task_runner_, &http_client_, - &network_, &dns_sd_, &http_server_, nullptr, &bluetooth_); + device_->Start(&config_store_, &task_runner_, &http_client_, &network_, + &dns_sd_, &http_server_, nullptr, &bluetooth_); for (const auto& cb : http_server_changed_cb_) cb.Run(http_server_); @@ -375,6 +366,7 @@ } TEST_F(WeaveBasicTest, Register) { + EXPECT_CALL(network_, OpenSslSocket(_, _, _, _)).WillRepeatedly(Return()); StartDevice(); auto draft = CreateDictionaryValue(kDeviceResource);