Fix NetworkSmsHandler to observe Manager and improve API

chromeos/network/network_sms_handler.cc

 // Copyright (c) 2012 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 "chromeos/network/network_sms_handler.h"
 
 #include <algorithm>
 #include <deque>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
+#include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_device_client.h"
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/gsm_sms_client.h"
 #include "chromeos/dbus/modem_messaging_client.h"
 #include "chromeos/dbus/sms_client.h"
 #include "dbus/object_path.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
-// Not exposed/exported.
 namespace {
+
+// Not exposed/exported:
 const char kSmscKey[] = "smsc";
 const char kValidityKey[] = "validity";
 const char kClassKey[] = "class";
 const char kIndexKey[] = "index";
 
 // Keys from ModemManager1
 const char kModemManager1NumberKey[] = "Number";
 const char kModemManager1TextKey[] = "Text";
 const char kModemManager1TimestampKey[] = "Timestamp";
+
+// Maximum number of messages stored for RequestUpdate(true).
+const size_t kMaxReceivedMessages = 100;

[Greg Spencer (Chromium), 2013/03/20 18:30:52]

This seems kind of low to me.  My phone defaults to 500 messages of history (and
I set it to 1000).

Or perhaps we shouldn't even provide a way to get old messages here and let
something else handle this that can provide persistence across sessions as well?

[stevenjb, 2013/03/25 19:51:10]

We don't really have a fully featured SMS system, and we don't persist them once
received. This represents the max number of messages retained in a session
(which would include messages received on connect since the previous connection
to the network). Given our limited UI, more than 100 seemed excessive. That
said, it's just a question of memory, so we could make this 1000 if we're
concerned about people loosing messages.

+
 }  // namespace
 
 namespace chromeos {
 
 // static
 const char NetworkSmsHandler::kNumberKey[] = "number";
 const char NetworkSmsHandler::kTextKey[] = "text";
 const char NetworkSmsHandler::kTimestampKey[] = "timestamp";
 
+static NetworkSmsHandler* g_network_sms_handler = NULL;
+
 class NetworkSmsHandler::NetworkSmsDeviceHandler {
  public:
   NetworkSmsDeviceHandler() {}
   virtual ~NetworkSmsDeviceHandler() {}
 
   virtual void RequestUpdate() = 0;
 };
 
 class NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler
     : public NetworkSmsHandler::NetworkSmsDeviceHandler {
@@ skipping 272 matching lines @@
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // NetworkSmsHandler
 
 NetworkSmsHandler::NetworkSmsHandler()
     : weak_ptr_factory_(this) {
 }
 
 NetworkSmsHandler::~NetworkSmsHandler() {
+  DBusThreadManager::Get()->GetShillManagerClient()->
+      RemovePropertyChangedObserver(this);
 }
 
-void NetworkSmsHandler::Init() {
-  // TODO(stevenjb): This code needs to monitor changes to Manager.Network
-  // so that devices added after Init() is called get added to device_handlers_.
-  // See: crbug.com/133416.
-
+void NetworkSmsHandler::InitShillDevices() {
+  // Add as an observer here so that new devices added after this call are
+  // recognized.
+  DBusThreadManager::Get()->GetShillManagerClient()->AddPropertyChangedObserver(
+      this);
   // Request network manager properties so that we can get the list of devices.
   DBusThreadManager::Get()->GetShillManagerClient()->GetProperties(
       base::Bind(&NetworkSmsHandler::ManagerPropertiesCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void NetworkSmsHandler::RequestUpdate() {
+// static
+void NetworkSmsHandler::Initialize() {
+  CHECK(!g_network_sms_handler);
+  g_network_sms_handler = new NetworkSmsHandler();
+  g_network_sms_handler->InitShillDevices();
+}
+
+// static
+void NetworkSmsHandler::Shutdown() {
+  CHECK(g_network_sms_handler);
+  delete g_network_sms_handler;
+  g_network_sms_handler = NULL;
+}
+
+// static
+NetworkSmsHandler* NetworkSmsHandler::Get() {
+  CHECK(g_network_sms_handler)
+      << "NetworkSmsHandler::Get() called before Initialize()";
+  return g_network_sms_handler;
+}
+
+void NetworkSmsHandler::RequestUpdate(bool request_existing) {
+  // If we already received messages and |request_existing| is true, send
+  // updates for existing messages.
+  for (ScopedVector<base::DictionaryValue>::iterator iter =
+           received_messages_.begin();
+       iter != received_messages_.end(); ++iter) {
+    base::DictionaryValue* message = *iter;
+    NotifyMessageReceived(*message);
+  }
+  // Request updates from each device.
   for (ScopedVector<NetworkSmsDeviceHandler>::iterator iter =
            device_handlers_.begin(); iter != device_handlers_.end(); ++iter) {
     (*iter)->RequestUpdate();
   }
 }
 
 void NetworkSmsHandler::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void NetworkSmsHandler::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void NetworkSmsHandler::OnPropertyChanged(const std::string& name,
+                                          const base::Value& value) {
+  if (name != flimflam::kDevicesProperty)
+    return;
+  const base::ListValue* devices = NULL;
+  if (!value.GetAsList(&devices) || !devices)
+    return;
+  UpdateDevices(devices);
+}
+
+// Private methods
+
 void NetworkSmsHandler::NotifyMessageReceived(
     const base::DictionaryValue& message) {
+  base::DictionaryValue* new_message = message.DeepCopy();
+  if (received_messages_.size() >= kMaxReceivedMessages)

[Greg Spencer (Chromium), 2013/03/20 18:30:52]

perhaps "while" instead of "if"?  If we allow changing the max dynamically
someday, that may be important.

[stevenjb, 2013/03/25 19:51:10]

This is the only place we add entries to received_messages_, so if() is
logically sufficient. 
I'll move this logic to an AddReceivedMessage() function to make that more
clear, with a comment that it is the only place messages should be added.
Looking more closely at the code, I think this should be split into
AddReceivedMessage and NotifyMessageReceived, with a helper function
MessageReceived() that calls both, I'll change that.

+    received_messages_.erase(received_messages_.begin());
+  received_messages_.push_back(new_message);
   FOR_EACH_OBSERVER(Observer, observers_, MessageReceived(message));
 }
 
 void NetworkSmsHandler::ManagerPropertiesCallback(
     DBusMethodCallStatus call_status,
     const base::DictionaryValue& properties) {
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
     LOG(ERROR) << "NetworkSmsHandler: Failed to get manager properties.";
     return;
   }
   const base::Value* value;
   if (!properties.GetWithoutPathExpansion(flimflam::kDevicesProperty, &value) ||
       value->GetType() != base::Value::TYPE_LIST) {
     LOG(ERROR) << "NetworkSmsHandler: No list value for: "
                << flimflam::kDevicesProperty;
     return;
   }
   const base::ListValue* devices = static_cast<const base::ListValue*>(value);
+  UpdateDevices(devices);
+}
+
+void NetworkSmsHandler::UpdateDevices(const base::ListValue* devices) {
   for (base::ListValue::const_iterator iter = devices->begin();
        iter != devices->end(); ++iter) {
     std::string device_path;
     (*iter)->GetAsString(&device_path);
     if (!device_path.empty()) {
       // Request device properties.
       VLOG(1) << "GetDeviceProperties: " << device_path;
       DBusThreadManager::Get()->GetShillDeviceClient()->GetProperties(
           dbus::ObjectPath(device_path),
           base::Bind(&NetworkSmsHandler::DevicePropertiesCallback,
@@ skipping 44 matching lines @@
             this, dbus_connection, object_path));
   } else {
     device_handlers_.push_back(
         new ModemManagerNetworkSmsDeviceHandler(
             this, dbus_connection, object_path));
   }
 }
 
 
 }  // namespace chromeos