Adding a NetworkProfileHandler used by ManagedNetworkConfigurationHandler.

chromeos/network/network_profile_handler.cc

Index: chromeos/network/network_profile_handler.cc
diff --git a/chromeos/network/network_profile_handler.cc b/chromeos/network/network_profile_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..49286d5a53f9a76b760a20fbfe736a7a22ebba59
--- /dev/null
+++ b/chromeos/network/network_profile_handler.cc
@@ -0,0 +1,227 @@
+// Copyright (c) 2013 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_profile_handler.h"
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/string_util.h"
+#include "base/values.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/dbus/shill_profile_client.h"
+#include "chromeos/network/network_profile_observer.h"
+#include "dbus/object_path.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+// TODO(pneubeck): remove once cros_system_api DEP is rolled!!!!!!!
+// !!!!!!!!!!!!
+namespace shill {
+const char kUserHashProperty[] = "UserHash";
+}
+
+namespace chromeos {
+
+namespace {
+
+NetworkProfileHandler* g_profile_handler_instance = NULL;

[stevenjb, 2013/04/26 21:00:49]

nit: for debugging ease I prefer not to put the global instance in an anon
namespace.

+
+bool ConvertListValueToStringVector(const base::ListValue& string_list,
+                                    std::vector<std::string>* result) {
+  for (size_t i = 0; i < string_list.GetSize(); ++i) {
+    std::string str;
+    if (!string_list.GetString(i, &str))
+      return false;
+    result->push_back(str);
+  }
+  return true;
+}
+
+void GetProfilePropertiesError(const std::string& profile_path,
+                               const std::string& error_name,
+                               const std::string& error_message) {

[stevenjb, 2013/04/26 21:00:49]

s/Get/Log

[pneubeck (no reviews), 2013/04/29 18:05:51]

Done.

+  LOG(ERROR) << "Error when requesting properties for profile "
+             << profile_path << ": " << error_message;
+}
+
+NetworkProfileHandler::ProfileList::const_iterator FindByPath(
+    const NetworkProfileHandler::ProfileList& profiles,

[stevenjb, 2013/04/26 21:00:49]

There's at least one case below where you really want a non-const iterator. If
you add a CompareByPath() method to NetworkProfile you could use std::find
instead of this and get either a const or non const iterator result.

[pneubeck (no reviews), 2013/04/29 18:05:51]

Done.

+    const std::string& path) {
+  for (NetworkProfileHandler::ProfileList::const_iterator it = profiles.begin();
+       it != profiles.end(); ++it) {
+    if (it->path == path)
+      return it;
+  }
+  return profiles.end();
+}
+
+}  // namespace
+
+// static
+NetworkProfileHandler* NetworkProfileHandler::Initialize() {
+  CHECK(!g_profile_handler_instance);
+  g_profile_handler_instance = new NetworkProfileHandler();
+  DBusThreadManager::Get()->GetShillManagerClient()->
+      AddPropertyChangedObserver(g_profile_handler_instance);
+  g_profile_handler_instance->RequestInitialProfileList();

[stevenjb, 2013/04/26 21:00:49]

Why not do these in the constructor?

[pneubeck (no reviews), 2013/04/29 18:05:51]

because of the derived Mock and Stub.
As many people would find it certainly confusing if the Stub would derive from
the actual implementation, the lengthy alternative seems to be:
- make ProfileHandler a pure interface
- call this implementation ..Base
- derive ..Impl from ..Base which un-/registers in the de-/constructor
- derive ..Stub from ..Base

[stevenjb, 2013/04/30 17:42:47]

I see. Typically we would use an Impl class and call:
g_profile_handler_instance = new NetworkProfileHandlerImpl()

I think you could easily do that without renaming the base class, and I think it
would be more clear.

+  return g_profile_handler_instance;
+}
+
+// static
+bool NetworkProfileHandler::IsInitialized() {
+  return g_profile_handler_instance;
+}
+
+// static
+void NetworkProfileHandler::Shutdown() {
+  CHECK(g_profile_handler_instance);
+  DBusThreadManager::Get()->GetShillManagerClient()->
+      RemovePropertyChangedObserver(g_profile_handler_instance);

[stevenjb, 2013/04/26 21:00:49]

In destructor?

+  delete g_profile_handler_instance;
+  g_profile_handler_instance = NULL;
+}
+
+// static
+NetworkProfileHandler* NetworkProfileHandler::Get() {
+  CHECK(g_profile_handler_instance);
+  return g_profile_handler_instance;
+}
+
+void NetworkProfileHandler::AddObserver(NetworkProfileObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void NetworkProfileHandler::RemoveObserver(NetworkProfileObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void NetworkProfileHandler::RequestInitialProfileList() {
+  // Request the initial profile list.
+  DBusThreadManager::Get()->GetShillManagerClient()->GetProperties(
+      base::Bind(&NetworkProfileHandler::GetManagerPropertiesCallback,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void NetworkProfileHandler::GetManagerPropertiesCallback(
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  if (DBUS_METHOD_CALL_FAILURE) {
+    LOG(ERROR) << "Error when requesting manager properties.";
+    return;
+  }
+
+  const base::Value* profiles = NULL;
+  properties.GetWithoutPathExpansion(flimflam::kProfilesProperty, &profiles);
+  DCHECK(profiles);

[stevenjb, 2013/04/26 21:00:49]

I'd rather not crash chrome if Shill fails to provide Profiles. We should
LOG(ERROR) and return instead.

[pneubeck (no reviews), 2013/04/29 18:05:51]

Done.

+  OnPropertyChanged(flimflam::kProfilesProperty, *profiles);
+}
+
+void NetworkProfileHandler::OnPropertyChanged(const std::string& name,
+                                              const base::Value& value) {
+  if (name != flimflam::kProfilesProperty)
+    return;
+
+  const base::ListValue* profiles_value = NULL;
+  value.GetAsList(&profiles_value);
+  DCHECK(profiles_value);
+
+  std::vector<std::string> new_profile_paths;
+  bool result = ConvertListValueToStringVector(*profiles_value,
+                                               &new_profile_paths);
+  DCHECK(result);
+
+  // Search for removed profiles.
+  std::vector<std::string> removed_profile_paths;
+  for (ProfileList::const_iterator it = profiles_.begin();
+       it != profiles_.end(); ++it) {
+    LOG(ERROR) << "  " << it->path;

[stevenjb, 2013/04/26 21:00:49]

remove

[pneubeck (no reviews), 2013/04/29 18:05:51]

Done.

+    if (std::find(new_profile_paths.begin(),
+                  new_profile_paths.end(),
+                  it->path) == new_profile_paths.end()) {
+      removed_profile_paths.push_back(it->path);
+    }
+  }
+
+  for (std::vector<std::string>::const_iterator it =
+           removed_profile_paths.begin();
+       it != removed_profile_paths.end(); ++it) {
+    RemoveProfile(*it);
+  }
+
+  for (std::vector<std::string>::const_iterator it = new_profile_paths.begin();
+       it != new_profile_paths.end(); ++it) {
+    // Skip known profiles. The associated userhash should never change.
+    if (GetProfileForPath(*it))
+      continue;
+
+    VLOG(2) << "Requesting properties of profile path " << *it << ".";
+    DBusThreadManager::Get()->GetShillProfileClient()->GetProperties(
+        dbus::ObjectPath(*it),
+        base::Bind(&NetworkProfileHandler::GetProfilePropertiesCallback,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   *it),
+        base::Bind(&GetProfilePropertiesError, *it));
+  }
+}
+
+void NetworkProfileHandler::GetProfilePropertiesCallback(
+    const std::string& profile_path,
+    const base::DictionaryValue& properties) {
+  std::string userhash;
+  properties.GetStringWithoutPathExpansion(shill::kUserHashProperty,
+                                           &userhash);
+
+  AddProfile(NetworkProfile(profile_path, userhash));
+}
+
+void NetworkProfileHandler::AddProfile(const NetworkProfile& profile) {
+  VLOG(2) << "Adding profile " << ProfileToString(&profile) << ".";
+  profiles_.push_back(profile);
+  FOR_EACH_OBSERVER(NetworkProfileObserver, observers_,
+                    OnProfileAdded(profiles_.back()));
+}
+
+void NetworkProfileHandler::RemoveProfile(const std::string& profile_path) {
+  VLOG(2) << "Removing profile for path " << profile_path << ".";
+  ProfileList::const_iterator found = FindByPath(profiles_, profile_path);
+  if (found == profiles_.end())
+    return;
+  NetworkProfile profile = *found;
+  // Workaround the deficiency of std::vector<>::erase(), which doesn't accept
+  // const_iterator.
+  profiles_.erase(profiles_.begin() + (found - profiles_.begin()));

[stevenjb, 2013/04/26 21:00:49]

Ew. See suggestion above for adding a comparator to NetworkProfile and using
std::find() instead of FindByPath.

[pneubeck (no reviews), 2013/04/29 18:05:51]

Done.

+  FOR_EACH_OBSERVER(NetworkProfileObserver, observers_,
+                    OnProfileRemoved(profile));
+}
+
+const NetworkProfile* NetworkProfileHandler::GetProfileForPath(
+    const std::string& profile_path) const {
+  ProfileList::const_iterator found = FindByPath(profiles_, profile_path);
+  if (found == profiles_.end())
+    return NULL;
+  return &*found;
+}
+
+const NetworkProfile* NetworkProfileHandler::GetProfileForUserhash(
+    const std::string& userhash) const {

[stevenjb, 2013/04/26 21:00:49]

This could also use std::find with a CompareByUserHash() operator.

[pneubeck (no reviews), 2013/04/29 18:05:51]

IMO, not worth the overhead of the comparator definition as above. Done.

+  for (NetworkProfileHandler::ProfileList::const_iterator it =
+           profiles_.begin();
+       it != profiles_.end(); ++it) {
+    if (it->userhash == userhash)
+      return &*it;
+  }
+  return NULL;
+}
+
+NetworkProfileHandler::NetworkProfileHandler()
+    : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+
+}
+
+NetworkProfileHandler::~NetworkProfileHandler() {
+
+}
+
+}  // namespace chromeos