Blimp Settings framework on the c++ side

blimp/client/core/settings/settings_feature.cc

 // Copyright 2016 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 "blimp/client/core/settings/settings_feature.h"
 
+#include "blimp/client/core/settings/settings.h"
 #include "blimp/common/create_blimp_message.h"
 #include "blimp/common/proto/blimp_message.pb.h"
 #include "blimp/common/proto/settings.pb.h"
 #include "net/base/net_errors.h"
 
 namespace blimp {
 namespace client {
 
-SettingsFeature::SettingsFeature() : record_whole_document_(false) {}
+SettingsFeature::SettingsFeature(Settings* settings)
+    : settings_(settings) {
+  if (settings_) {

[David Trainor- moved to gerrit, 2016/10/05 23:46:21]

Remove {} if single line after if.  Same below.

[Menglin, 2016/10/06 22:36:38]

don't need the observer setup since SettingsFeature doesn't have to subclass
SettingsObserver after remove OnRecordWholeDocumentChanged

+    settings_->AddObserver(this);
+  }
+}
 
-SettingsFeature::~SettingsFeature() {}
+SettingsFeature::~SettingsFeature() {
+  if (settings_) {
+    settings_->RemoveObserver(this);
+  }
+}
 
 void SettingsFeature::set_outgoing_message_processor(
     std::unique_ptr<BlimpMessageProcessor> processor) {
   outgoing_message_processor_ = std::move(processor);
 }
 
-void SettingsFeature::SetRecordWholeDocument(bool record_whole_document) {
-  if (record_whole_document_ == record_whole_document)
-    return;
-
-  record_whole_document_ = record_whole_document;
-
+void SettingsFeature::OnRecordWholeDocumentChanged(bool enable) {

[David Trainor- moved to gerrit, 2016/10/05 23:46:21]

We shouldn't need this.  IIUC we only expect this to work after restarts.

[Menglin, 2016/10/06 22:36:38]

OK. I'm removing this method, in the mean time, SettingsFeature doesn't have to
subclass SettingsObserver.

My understanding is when record_whole_document_ is changed, restart will be
triggered, and the message will be sent in PushSettings after the restart, so we
don't need this method. cmiiw

   EngineSettingsMessage* engine_settings;
   std::unique_ptr<BlimpMessage> message = CreateBlimpMessage(&engine_settings);
-  engine_settings->set_record_whole_document(record_whole_document_);
+  engine_settings->set_record_whole_document(enable);
   outgoing_message_processor_->ProcessMessage(std::move(message),
                                               net::CompletionCallback());
 }
 
 void SettingsFeature::SendUserAgentOSVersionInfo(const std::string& osVersion) {

[David Trainor- moved to gerrit, 2016/10/05 23:46:21]

Add a line saying remove this method and tie it to your TODO down below.  We
won't need this once we send it with the initial settings values.

[Menglin, 2016/10/06 22:36:38]

Done.

   EngineSettingsMessage* engine_settings;
   std::unique_ptr<BlimpMessage> message = CreateBlimpMessage(&engine_settings);
   engine_settings->set_client_os_info(osVersion);
   outgoing_message_processor_->ProcessMessage(std::move(message),
                                               net::CompletionCallback());
 }
 
 void SettingsFeature::ProcessMessage(std::unique_ptr<BlimpMessage> message,
                                      const net::CompletionCallback& callback) {
   // We don't receive any messages from the engine yet.
   NOTREACHED() << "Invalid settings message received from the engine.";
   callback.Run(net::OK);
 }
 
+void SettingsFeature::PushSettings() {
+  // TODO(mlliu): set the user agent on the proto as well after moving
+  // blimp/client/app/user_agent.* to this directory (http://crbug.com/652032).
+  EngineSettingsMessage* engine_settings;
+  std::unique_ptr<BlimpMessage> message = CreateBlimpMessage(&engine_settings);
+  engine_settings->set_record_whole_document(
+      settings_->record_whole_document());
+  outgoing_message_processor_->ProcessMessage(std::move(message),
+                                              net::CompletionCallback());
+}
+
 }  // namespace client
 }  // namespace blimp