Fix NetworkSmsHandler to observe Manager and improve API

BUG=133416

Review URL: https://chromiumcodereview.appspot.com/12669004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@215783 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 136 insertions, 47 deletions

diff --git a/chromeos/dbus/shill_property_changed_observer.h b/chromeos/dbus/shill_property_changed_observer.h
index d27482e..7ee9373 100644
--- a/chromeos/dbus/shill_property_changed_observer.h
+++ b/chromeos/dbus/shill_property_changed_observer.h
@@ -17,9 +17,11 @@ namespace chromeos {
 // sent from Shill.
 class ShillPropertyChangedObserver {
  public:
-  virtual ~ShillPropertyChangedObserver() {}
   virtual void OnPropertyChanged(const std::string& name,
                                  const base::Value& value) = 0;
+
+ protected:
+  virtual ~ShillPropertyChangedObserver() {}
 };
 
 }  // namespace chromeos
diff --git a/chromeos/network/network_handler.cc b/chromeos/network/network_handler.cc
index 272a1de..fc6896b 100644
--- a/chromeos/network/network_handler.cc
+++ b/chromeos/network/network_handler.cc
@@ -13,6 +13,7 @@
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_profile_observer.h"
+#include "chromeos/network/network_sms_handler.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_state_handler_observer.h"
 
@@ -32,6 +33,7 @@ NetworkHandler::NetworkHandler() {
   managed_network_configuration_handler_.reset(
       new ManagedNetworkConfigurationHandler());
   network_connection_handler_.reset(new NetworkConnectionHandler());
+  network_sms_handler_.reset(new NetworkSmsHandler());
   geolocation_handler_.reset(new GeolocationHandler());
 }
 
@@ -49,6 +51,7 @@ void NetworkHandler::Init() {
       network_configuration_handler_.get());
   network_connection_handler_->Init(network_state_handler_.get(),
                                     network_configuration_handler_.get());
+  network_sms_handler_->Init();
   geolocation_handler_->Init();
 }
 
@@ -103,6 +106,10 @@ NetworkConnectionHandler* NetworkHandler::network_connection_handler() {
   return network_connection_handler_.get();
 }
 
+NetworkSmsHandler* NetworkHandler::network_sms_handler() {
+  return network_sms_handler_.get();
+}
+
 GeolocationHandler* NetworkHandler::geolocation_handler() {
   return geolocation_handler_.get();
 }
diff --git a/chromeos/network/network_handler.h b/chromeos/network/network_handler.h
index b25ab1b..9cc2d46 100644
--- a/chromeos/network/network_handler.h
+++ b/chromeos/network/network_handler.h
@@ -18,6 +18,7 @@ class NetworkConnectionHandler;
 class NetworkDeviceHandler;
 class NetworkProfileHandler;
 class NetworkStateHandler;
+class NetworkSmsHandler;
 
 // Class for handling initialization and access to chromeos network handlers.
 // This class should NOT be used in unit tests. Instead, construct individual
@@ -45,6 +46,7 @@ class CHROMEOS_EXPORT NetworkHandler {
   NetworkConfigurationHandler* network_configuration_handler();
   ManagedNetworkConfigurationHandler* managed_network_configuration_handler();
   NetworkConnectionHandler* network_connection_handler();
+  NetworkSmsHandler* network_sms_handler();
   GeolocationHandler* geolocation_handler();
 
  private:
@@ -61,6 +63,7 @@ class CHROMEOS_EXPORT NetworkHandler {
   scoped_ptr<ManagedNetworkConfigurationHandler>
       managed_network_configuration_handler_;
   scoped_ptr<NetworkConnectionHandler> network_connection_handler_;
+  scoped_ptr<NetworkSmsHandler> network_sms_handler_;
   scoped_ptr<GeolocationHandler> geolocation_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkHandler);
diff --git a/chromeos/network/network_sms_handler.cc b/chromeos/network/network_sms_handler.cc
index f70314b0..986317d 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;
+
 }  // namespace
 
 namespace chromeos {
@@ -61,7 +67,7 @@ class NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler
   void SmsReceivedCallback(uint32 index, bool complete);
   void GetCallback(uint32 index, const base::DictionaryValue& dictionary);
   void DeleteMessages();
-  void NotifyMessageReceived(const base::DictionaryValue& dictionary);
+  void MessageReceived(const base::DictionaryValue& dictionary);
 
   NetworkSmsHandler* host_;
   std::string dbus_connection_;
@@ -111,7 +117,7 @@ void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::ListCallback(
     base::DictionaryValue* message = NULL;
     if (!(*iter)->GetAsDictionary(&message))
       continue;
-    NotifyMessageReceived(*message);
+    MessageReceived(*message);
     double index = 0;
     if (message->GetDoubleWithoutPathExpansion(kIndexKey, &index))
       delete_queue_.push_back(static_cast<uint32>(index));
@@ -154,19 +160,19 @@ ModemManagerNetworkSmsDeviceHandler::SmsReceivedCallback(
 void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::GetCallback(
     uint32 index,
     const base::DictionaryValue& dictionary) {
-  NotifyMessageReceived(dictionary);
+  MessageReceived(dictionary);
   delete_queue_.push_back(index);
   if (!deleting_messages_)
     DeleteMessages();
 }
 
 void NetworkSmsHandler::
-ModemManagerNetworkSmsDeviceHandler::NotifyMessageReceived(
+ModemManagerNetworkSmsDeviceHandler::MessageReceived(
     const base::DictionaryValue& dictionary) {
   // The keys of the ModemManager.Modem.Gsm.SMS interface match the
   // exported keys, so the dictionary used as a notification argument
   // unchanged.
-  host_->NotifyMessageReceived(dictionary);
+  host_->MessageReceived(dictionary);
 }
 
 class NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler
@@ -184,7 +190,7 @@ class NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler
   void GetCallback(const base::DictionaryValue& dictionary);
   void DeleteMessages();
   void GetMessages();
-  void NotifyMessageReceived(const base::DictionaryValue& dictionary);
+  void MessageReceived(const base::DictionaryValue& dictionary);
 
   NetworkSmsHandler* host_;
   std::string dbus_connection_;
@@ -299,12 +305,12 @@ ModemManager1NetworkSmsDeviceHandler::SmsReceivedCallback(
 
 void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::GetCallback(
     const base::DictionaryValue& dictionary) {
-  NotifyMessageReceived(dictionary);
+  MessageReceived(dictionary);
   GetMessages();
 }
 
 void NetworkSmsHandler::
-ModemManager1NetworkSmsDeviceHandler::NotifyMessageReceived(
+ModemManager1NetworkSmsDeviceHandler::MessageReceived(
     const base::DictionaryValue& dictionary) {
   // The keys of the ModemManager1.SMS interface do not match the
   // exported keys, so a new dictionary is created with the expected
@@ -320,7 +326,7 @@ ModemManager1NetworkSmsDeviceHandler::NotifyMessageReceived(
   if (dictionary.GetStringWithoutPathExpansion(kModemManager1TimestampKey,
                                                &timestamp))
     new_dictionary.SetString(kTimestampKey, timestamp);
-  host_->NotifyMessageReceived(new_dictionary);
+  host_->MessageReceived(new_dictionary);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -331,20 +337,31 @@ 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.
-
+  // 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() {
+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,11 +376,36 @@ 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::AddReceivedMessage(
+    const base::DictionaryValue& message) {
+  base::DictionaryValue* new_message = message.DeepCopy();
+  if (received_messages_.size() >= kMaxReceivedMessages)
+    received_messages_.erase(received_messages_.begin());
+  received_messages_.push_back(new_message);
+}
+
 void NetworkSmsHandler::NotifyMessageReceived(
     const base::DictionaryValue& message) {
   FOR_EACH_OBSERVER(Observer, observers_, MessageReceived(message));
 }
 
+void NetworkSmsHandler::MessageReceived(const base::DictionaryValue& message) {
+  AddReceivedMessage(message);
+  NotifyMessageReceived(message);
+}
+
 void NetworkSmsHandler::ManagerPropertiesCallback(
     DBusMethodCallStatus call_status,
     const base::DictionaryValue& properties) {
@@ -379,6 +421,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;
diff --git a/chromeos/network/network_sms_handler.h b/chromeos/network/network_sms_handler.h
index 5bbe09e..da4a10e 100644
--- a/chromeos/network/network_sms_handler.h
+++ b/chromeos/network/network_sms_handler.h
@@ -10,14 +10,20 @@
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
-#include "base/values.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
+#include "chromeos/dbus/shill_property_changed_observer.h"
+
+namespace base {
+class DictionaryValue;
+class ListValue;
+class Value;
+}
 
 namespace chromeos {
 
 // Class to watch sms without Libcros.
-class CHROMEOS_EXPORT NetworkSmsHandler {
+class CHROMEOS_EXPORT NetworkSmsHandler : public ShillPropertyChangedObserver {
  public:
   static const char kNumberKey[];
   static const char kTextKey[];
@@ -32,32 +38,51 @@ class CHROMEOS_EXPORT NetworkSmsHandler {
     virtual void MessageReceived(const base::DictionaryValue& message) = 0;
   };
 
-  NetworkSmsHandler();
-  ~NetworkSmsHandler();
-
-  // Requests the devices from the netowork manager, sets up observers, and
-  // requests the initial list of messages. Any observers that wish to be
-  // notified with initial messages should be added before calling this.
-  void Init();
+  virtual ~NetworkSmsHandler();
 
-  // Requests an immediate check for new messages.
-  void RequestUpdate();
+  // Requests an immediate check for new messages. If |request_existing| is
+  // true then also requests to be notified for any already received messages.
+  void RequestUpdate(bool request_existing);
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // ShillPropertyChangedObserver
+  virtual void OnPropertyChanged(const std::string& name,
+                                 const base::Value& value) OVERRIDE;
+
  private:
+  friend class NetworkHandler;
+  friend class NetworkSmsHandlerTest;
+
   class NetworkSmsDeviceHandler;
   class ModemManagerNetworkSmsDeviceHandler;
   class ModemManager1NetworkSmsDeviceHandler;
 
-  // Called from NetworkSmsDeviceHandler when a message is received.
+  NetworkSmsHandler();
+
+  // Requests the devices from the network manager, sets up observers, and
+  // requests the initial list of messages.
+  void Init();
+
+  // Adds |message| to the list of received messages. If the length of the
+  // list exceeds the maximum number of retained messages, erase the least
+  // recently received message.
+  void AddReceivedMessage(const base::DictionaryValue& message);
+
+  // Notify observers that |message| was received.
   void NotifyMessageReceived(const base::DictionaryValue& message);
 
+    // Called from NetworkSmsDeviceHandler when a message is received.
+  void MessageReceived(const base::DictionaryValue& message);
+
   // Callback to handle the manager properties with the list of devices.
   void ManagerPropertiesCallback(DBusMethodCallStatus call_status,
                                  const base::DictionaryValue& properties);
 
+  // Requests properties for each entry in |devices|.
+  void UpdateDevices(const base::ListValue* devices);
+
   // Callback to handle the device properties for |device_path|.
   // A NetworkSmsDeviceHandler will be instantiated for each cellular device.
   void DevicePropertiesCallback(const std::string& device_path,
@@ -66,6 +91,7 @@ class CHROMEOS_EXPORT NetworkSmsHandler {
 
   ObserverList<Observer> observers_;
   ScopedVector<NetworkSmsDeviceHandler> device_handlers_;
+  ScopedVector<base::DictionaryValue> received_messages_;
   base::WeakPtrFactory<NetworkSmsHandler> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkSmsHandler);
diff --git a/chromeos/network/network_sms_handler_unittest.cc b/chromeos/network/network_sms_handler_unittest.cc
index 4831538..c7eaac2 100644
--- a/chromeos/network/network_sms_handler_unittest.cc
+++ b/chromeos/network/network_sms_handler_unittest.cc
@@ -55,6 +55,11 @@ class NetworkSmsHandlerTest : public testing::Test {
   virtual ~NetworkSmsHandlerTest() {}
 
   virtual void SetUp() OVERRIDE {
+    // Append '--sms-test-messages' to the command line to tell
+    // SMSClientStubImpl to generate a series of test SMS messages.
+    CommandLine* command_line = CommandLine::ForCurrentProcess();
+    command_line->AppendSwitch(chromeos::switches::kSmsTestMessages);
+
     // Initialize DBusThreadManager with a stub implementation.
     DBusThreadManager::InitializeWithStub();
     ShillManagerClient::TestInterface* manager_test =
@@ -66,35 +71,35 @@ class NetworkSmsHandlerTest : public testing::Test {
     ASSERT_TRUE(device_test);
     device_test->AddDevice("stub_cellular_device2", flimflam::kTypeCellular,
                            "/org/freedesktop/ModemManager1/stub/0");
+
+    // This relies on the stub dbus implementations for ShillManagerClient,
+    // ShillDeviceClient, GsmSMSClient, ModemMessagingClient and SMSClient.
+    // Initialize a sms handler. The stub dbus clients will not send the
+    // first test message until RequestUpdate has been called.
+    network_sms_handler_.reset(new NetworkSmsHandler());
+    network_sms_handler_->Init();
+    test_observer_.reset(new TestObserver());
+    network_sms_handler_->AddObserver(test_observer_.get());
+    network_sms_handler_->RequestUpdate(true);
+    message_loop_.RunUntilIdle();
   }
 
   virtual void TearDown() OVERRIDE {
+    network_sms_handler_.reset();
     DBusThreadManager::Shutdown();
   }
 
  protected:
   base::MessageLoopForUI message_loop_;
+  scoped_ptr<NetworkSmsHandler> network_sms_handler_;
+  scoped_ptr<TestObserver> test_observer_;
 };
 
 TEST_F(NetworkSmsHandlerTest, SmsHandlerDbusStub) {
-  // Append '--sms-test-messages' to the command line to tell SMSClientStubImpl
-  // to generate a series of test SMS messages.
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
-  command_line->AppendSwitch(chromeos::switches::kSmsTestMessages);
-
-  // This relies on the stub dbus implementations for ShillManagerClient,
-  // ShillDeviceClient, GsmSMSClient, ModemMessagingClient and SMSClient.
-  // Initialize a sms handler. The stub dbus clients will not send the
-  // first test message until RequestUpdate has been called.
-  scoped_ptr<NetworkSmsHandler> sms_handler(new NetworkSmsHandler());
-  scoped_ptr<TestObserver> test_observer(new TestObserver());
-  sms_handler->AddObserver(test_observer.get());
-  sms_handler->Init();
-  message_loop_.RunUntilIdle();
-  EXPECT_EQ(test_observer->message_count(), 0);
+  EXPECT_EQ(test_observer_->message_count(), 0);
 
   // Test that no messages have been received yet
-  const std::set<std::string>& messages(test_observer->messages());
+  const std::set<std::string>& messages(test_observer_->messages());
   // Note: The following string corresponds to values in
   // ModemMessagingClientStubImpl and SmsClientStubImpl.
   // TODO(stevenjb): Use a TestInterface to set this up to remove dependency.
@@ -102,10 +107,10 @@ TEST_F(NetworkSmsHandlerTest, SmsHandlerDbusStub) {
   EXPECT_EQ(messages.find(kMessage1), messages.end());
 
   // Test for messages delivered by signals.
-  test_observer->ClearMessages();
-  sms_handler->RequestUpdate();
+  test_observer_->ClearMessages();
+  network_sms_handler_->RequestUpdate(false);
   message_loop_.RunUntilIdle();
-  EXPECT_GE(test_observer->message_count(), 1);
+  EXPECT_GE(test_observer_->message_count(), 1);
   EXPECT_NE(messages.find(kMessage1), messages.end());
 }