Fix NetworkSmsHandler to observe Manager and improve API

chromeos/network/network_sms_handler.cc

Index: chromeos/network/network_sms_handler.cc
diff --git a/chromeos/network/network_sms_handler.cc b/chromeos/network/network_sms_handler.cc
index f70314b08f8251242794c8231e31b8b5fecc0c88..4f32865f1b028a293088ee5b3e14ec9f89584367 100644
--- a/chromeos/network/network_sms_handler.cc
+++ b/chromeos/network/network_sms_handler.cc
@@ -10,6 +10,7 @@
 #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"
@@ -19,8 +20,9 @@
 #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";
@@ -30,6 +32,10 @@ const char kIndexKey[] = "index";
 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 {
@@ -39,6 +45,8 @@ 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() {}
@@ -331,20 +339,52 @@ NetworkSmsHandler::NetworkSmsHandler()
 }
 
 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();
@@ -359,8 +399,24 @@ 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));
 }
 
@@ -379,6 +435,10 @@ void NetworkSmsHandler::ManagerPropertiesCallback(
     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;