This adds a private extension API to use for simple networking

chrome/browser/chromeos/extensions/networking_private_api.cc

Index: chrome/browser/chromeos/extensions/networking_private_api.cc
diff --git a/chrome/browser/chromeos/extensions/networking_private_api.cc b/chrome/browser/chromeos/extensions/networking_private_api.cc
new file mode 100644
index 0000000000000000000000000000000000000000..92b00cf282def8ebad522b5034be032ac355fee1
--- /dev/null
+++ b/chrome/browser/chromeos/extensions/networking_private_api.cc
@@ -0,0 +1,309 @@
+// 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 "chrome/browser/chromeos/extensions/networking_private_api.h"
+
+#include "base/bind_helpers.h"
+#include "chrome/browser/chromeos/extensions/networking_private_api_factory.h"
+#include "chrome/browser/extensions/extension_function_registry.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/onc/onc_constants.h"
+#include "chromeos/network/onc/onc_signature.h"
+#include "chromeos/network/onc/onc_translation_tables.h"
+#include "chromeos/network/onc/onc_translator.h"
+#include "dbus/object_path.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {
+
+// An error returned when no valid services were found.
+const char kInvalidResponseError[] = "Error.invalidResponse";
+
+// This creates a new ONC dictionary that only contains the information we're
+// interested in passing on to JavaScript.
+base::DictionaryValue* CreateFilteredResult(
+    const base::DictionaryValue& properties) {
+  scoped_ptr<base::DictionaryValue> onc_properties(
+      onc::TranslateShillServiceToONCPart(
+          properties,
+          &onc::kNetworkConfigurationSignature));
+
+  // Now we filter it so we only include properties that we care about for this
+  // interface.
+  static const char* const desired_fields[] = {
+      onc::network_config::kWiFi,
+      onc::network_config::kName,
+      onc::network_config::kGUID,
+      onc::network_config::kType,
+      onc::network_config::kConnectionState,
+  };
+
+  scoped_ptr<base::DictionaryValue> filtered_result(new base::DictionaryValue);
+  for (size_t i = 0; i < arraysize(desired_fields); ++i) {
+    base::Value* value;
+    if (onc_properties->Get(desired_fields[i], &value))
+      filtered_result->Set(desired_fields[i], value->DeepCopy());
+  }
+  return filtered_result.release();
+}
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// NetworkingGetPropertiesFunction
+
+NetworkingGetPropertiesFunction::~NetworkingGetPropertiesFunction() {
+}
+
+bool NetworkingGetPropertiesFunction::RunImpl() {
+  std::string service_path;
+  if (!args_->GetString(0, &service_path))

[not at google - send to devlin, 2013/01/23 22:37:52]

Do you know about the JSON schema compiler? Ideally you should use it, but maybe
there's a reason you haven't.

The compiler generates all the Value boilerplate here, as well as making the
resource file, extension_api.cc, and RegisterFunction boilerplate unnecessary.

List networkingPrivate.json in api.gyp and set the necessary compiler_options
(see chrome/common/extensions/api/file_browser_handler_internal.json), then here
you'd have

scoped_ptr<api::GetProperties::Params> params =
    api::GetProperties::Params::Create(*args_);
EXTENSION_FUNCTION_VALIDATE(params);

DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
    dbus::ObjectPath(params->networkGuid),
    base::Bind(&NetworkingGetPropertiesFunction::ResultCallback, this));
return true;

[Greg Spencer (Chromium), 2013/01/31 16:40:36]

Well, I didn't know about it until I had already coded it this way. Looking at
the docs, it seemed as though either way was acceptable, but as using the schema
compiler is the "newer" way, I've rewritten it to use that.  I looked at moving
it to WebIDL, but that didn't seem to have all the capabilities I needed.

[not at google - send to devlin, 2013/02/01 19:09:42]

NOTE(chrome-extensions-team): we need to update the docs.

+    return false;
+
+  DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
+      dbus::ObjectPath(service_path), base::Bind(
+          &NetworkingGetPropertiesFunction::ResultCallback, this));

[not at google - send to devlin, 2013/01/23 22:37:52]

nit: line arguments vertically (like above)

[Greg Spencer (Chromium), 2013/01/31 16:40:36]

Done.

+  return true;
+}
+
+void NetworkingGetPropertiesFunction::ResultCallback(
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& result) {
+  scoped_ptr<base::DictionaryValue> filtered_result(
+      CreateFilteredResult(result));
+
+  SetResult(filtered_result.release());

[not at google - send to devlin, 2013/01/23 22:37:52]

And here, you'd make CreateFilteredResult return the generate type data, like

scoped_ptr<api::Properties> CreateFilteredResult(...) {}

then use like

result_ = api::GetProperties::Result::Create(
    CreateFilteredResult(...).release());

or whatever it is. Something along those lines that makes sense structurally.

This comment and the one above apply to all API bindings, and should make this
stuff a lot simpler.

By the way, if in doubt you will be able to see the code that got generate for
your API in out/Debug/gen/chrome/common/extensions/api/networking_private.h

+  SendResponse(true);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// NetworkingGetVisibleNetworksFunction::ResultList
+class NetworkingGetVisibleNetworksFunction::ResultList
+    : public base::RefCounted<ResultList> {

[not at google - send to devlin, 2013/01/23 22:37:52]

does this really need to be refcounted?

[Greg Spencer (Chromium), 2013/01/31 16:40:36]

Yes, it's kind of the reason this class exists: it's assembling the results of
multiple async calls to Shill, and so each callback needs a reference to the
list, and reference counting is the easiest way to assure that they all
disappear automatically when the callback objects go away.

[not at google - send to devlin, 2013/02/01 19:09:42]

Ok, cool, you're right. However I think I asked the wrong question here: the
problem as you say is that there are multiple async calls. However, I think a
cleaner approach to solving this problem is making it a single object's
responsibility to mediate this rather than a combination of two classes
collaborating (ResultList and NetworkingGetVisibleNetworksFunction). I imagine
it would look something like:

class ResultList : public base::RefCounted<ResultList> {
 public:
  ResultList(services, shill_service_client, object_arg_thingy, callback)
      : callback_(callback) {
    for (service : services) {
      shill_service_client->GetProperties(
          object_arg_thingy,
          base::Bind(&OnGetPropertiesResponse, this));
    }
  }

 private:
  friend class base::RefCounted<ResultList>;

  void OnGetPropertiesResult(...) {
    DCHECK(BrowserThread::CurrentlyOn(UI))  // or whatever
    result_.Append(...);
  }

  ~ResultList() {
     callback_.Run(result_);
  }

  result_;
};

the class could probably do with a different name then.

And yeah, the |callback| arg would basically be just SetResult and a
SendResponse.

[Greg Spencer (Chromium), 2013/02/01 23:35:55]

OK, I like this a little better: at least we can get rid of the count, and use
the refcount instead.

But why does the ResultList need to be the thing that calls GetProperties? 
Seems like once all the callbacks disappear, all of the references will be gone,
and it just needs to call a callback, so the ResultList could just take the
callback as an argument, and the rest of the code can stay the same...

I've done it that way (because it's easier to do), let me know if that's
sufficient.

Also, realize that this implementation is calling Shill directly because we
didn't want to make this API addition dependent upon other network code changes
(to avoid making this task dependent on the network code changes being ready for
26), so this implementation is likely to be short-lived.  As soon as the network
code changes are in (and they almost are), then this will use the new
NetworkStateHandler API instead of calling Shill directly, and all of these
shenanigans with the collection of async results will go away.

[not at google - send to devlin, 2013/02/02 00:11:32]

Well the reason is like I said, at the moment it's the responsibility of two
classes to gather the results and the control flow bounces back and forth. I
would find it much easier to follow if all of the logic and all of the data were
in a single place - here that means within ResultList. There would be no methods
exposed on ResultList at all, everything private. Law of demeter etc etc.

You could probably even make this class declared within an anonymous namespace
and not have a presence in the header file at all. In fact you might be able to
do it now anyway.
If it really is all going to go away, and ResultList really will be entirely
unnecessary, then it seems ok leave well enough alone and move on. However, I've
been bitten almost every time by somebody claiming they'll fix something because
things are about to change, and then it never happens.

But alright. Change my assumptions.

[Greg Spencer (Chromium), 2013/02/02 01:06:15]

It wasn't too hard to do, so I went ahead and did it.

[I couldn't have removed the declaration from the header file before because the
class was used as an argument to some of the callbacks]

+ public:
+  explicit ResultList(int count);
+
+  int count() const { return count_; }
+  void decrement() { --count_; }
+  base::ListValue* release() { return list_.release(); }
+  void Append(base::Value* value);
+
+ private:
+  friend class base::RefCounted<ResultList>;
+  ~ResultList();
+
+  scoped_ptr<base::ListValue> list_;
+  int count_;
+
+  DISALLOW_COPY_AND_ASSIGN(ResultList);
+};
+
+NetworkingGetVisibleNetworksFunction::ResultList::ResultList(int count)
+    : list_(new base::ListValue), count_(count) {
+}
+
+NetworkingGetVisibleNetworksFunction::ResultList::~ResultList() {
+}
+
+void NetworkingGetVisibleNetworksFunction::ResultList::Append(
+    base::Value* value) {
+  list_->Append(value);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// NetworkingGetVisibleNetworksFunction
+
+NetworkingGetVisibleNetworksFunction::~NetworkingGetVisibleNetworksFunction() {
+}
+
+bool NetworkingGetVisibleNetworksFunction::RunImpl() {
+  std::string network_type;
+  if (!args_->GetString(0, &network_type))
+    return false;
+
+  DBusThreadManager::Get()->GetShillManagerClient()->GetProperties(
+      base::Bind(
+          &NetworkingGetVisibleNetworksFunction::ManagerPropertiesCallback,
+          this,
+          network_type));
+  return true;
+}
+
+// For each of the available services, fire off a request for its properties.
+void NetworkingGetVisibleNetworksFunction::ManagerPropertiesCallback(
+    const std::string& network_type,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& result) {
+  const base::ListValue* available_services;
+  if (!result.GetList(flimflam::kServicesProperty, &available_services)) {
+    LOG(ERROR)
+        << "ShillManagerClient::GetProperties returned malformed service list.";
+    error_ = kInvalidResponseError;
+    SendResponse(false);
+    return;
+  }
+  scoped_refptr<ResultList> result_list(
+      new ResultList(available_services->GetSize()));
+  // If there just are no services, return an empty list.
+  if (available_services->GetSize() == 0) {
+    SetResult(result_list->release());
+    SendResponse(true);
+    return;
+  }
+  for (base::ListValue::const_iterator iter = available_services->begin();
+       iter != available_services->end(); ++iter) {
+    std::string service_path;
+    if (!(*iter)->GetAsString(&service_path)) {
+      LOG(ERROR)
+          << "ShillManagerClient::GetProperties returned malformed service.";
+      result_list->decrement();
+      continue;
+    }
+    DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
+        dbus::ObjectPath(service_path),
+        base::Bind(
+            &NetworkingGetVisibleNetworksFunction::ServicePropertiesCallback,
+            this,
+            service_path,
+            network_type,
+            result_list));
+  }
+  // All of the service paths were malformed, so we fail.
+  if (result_list->count() == 0) {
+    error_ = kInvalidResponseError;
+    SendResponse(false);
+  }
+}
+
+// If this network is of the appropriate type, add it to the results and
+// decrement the count.  If the count hits zero, then send the result.
+void NetworkingGetVisibleNetworksFunction::ServicePropertiesCallback(
+    const std::string& service_path,
+    const std::string& network_type,
+    scoped_refptr<ResultList> result_list,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& result) {
+  if (call_status == DBUS_METHOD_CALL_SUCCESS) {
+    scoped_ptr<base::DictionaryValue> filtered_result(
+        CreateFilteredResult(result));
+
+    std::string onc_type;
+    if (filtered_result->GetString(onc::network_config::kType, &onc_type) &&
+        (onc_type == network_type ||
+         network_type == onc::network_type::kAllTypes)) {
+          // TODO(gspencer): For now the "guid" we send back is going to look
+          // remarkably like the service path. Once this code starts using the
+          // NetworkStateHandler instead of Shill directly, we should remove
+          // this line so that we're sending back the actual GUID. The
+          // JavaScript shouldn't care: this ID is opaque to it, and it
+          // shouldn't store it anywhere.
+          filtered_result->SetString(onc::network_config::kGUID, service_path);
+
+          result_list->Append(filtered_result.release());
+    }
+  }
+  // We must decrement the count each time we get a callback, regardless of the
+  // outcome, or we'll never send a response.
+  result_list->decrement();
+  if (result_list->count() == 0) {
+    SetResult(result_list->release());
+    SendResponse(true);
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// NetworkingRequestConnectFunction
+
+NetworkingRequestConnectFunction::~NetworkingRequestConnectFunction() {
+}
+
+void  NetworkingRequestConnectFunction::ConnectRequestSuccess() {
+  SendResponse(true);
+}
+
+void NetworkingRequestConnectFunction::ConnectRequestFailed(
+    const std::string& errorName,

[not at google - send to devlin, 2013/01/23 22:37:52]

error_name not errorName (same below etc)

[Greg Spencer (Chromium), 2013/01/31 16:40:36]

Done.

+    const std::string& errorMessage) {
+  error_ = errorName;
+  SendResponse(false);
+}
+
+bool NetworkingRequestConnectFunction::RunImpl() {
+  std::string guid;
+  if (!args_->GetString(0, &guid))
+    return false;
+
+  // TODO(gspencer): For now, the "guid" we receive from the JavaScript is going
+  // to be the service path.  Fix this so it actually looks up the service path
+  // from the GUID once we're using the NetworkStateHandler.
+  std::string service_path = guid;
+
+  DBusThreadManager::Get()->GetShillServiceClient()->Connect(
+      dbus::ObjectPath(service_path),
+      base::Bind(&NetworkingRequestConnectFunction::ConnectRequestSuccess,
+                 this),
+      base::Bind(&NetworkingRequestConnectFunction::ConnectRequestFailed,
+                 this));
+  return true;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// NetworkingRequestDisconnectFunction
+
+NetworkingRequestDisconnectFunction::~NetworkingRequestDisconnectFunction() {
+}
+
+void  NetworkingRequestDisconnectFunction::DisconnectRequestSuccess() {
+  SendResponse(true);
+}
+
+void NetworkingRequestDisconnectFunction::DisconnectRequestFailed(
+    const std::string& errorName,
+    const std::string& errorMessage) {
+  error_ = errorName;
+  SendResponse(false);
+}
+
+bool NetworkingRequestDisconnectFunction::RunImpl() {
+  std::string service_path;
+  if (!args_->GetString(0, &service_path))
+    return false;
+
+  DBusThreadManager::Get()->GetShillServiceClient()->Connect(
+      dbus::ObjectPath(service_path),
+      base::Bind(&NetworkingRequestDisconnectFunction::DisconnectRequestSuccess,
+                 this),
+      base::Bind(&NetworkingRequestDisconnectFunction::DisconnectRequestFailed,
+                 this));
+  return true;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// NetworkingPrivateAPI
+
+NetworkingPrivateAPI::NetworkingPrivateAPI(Profile* profile) {
+  ExtensionFunctionRegistry* registry =
+      ExtensionFunctionRegistry::GetInstance();
+  registry->RegisterFunction<NetworkingGetPropertiesFunction>();
+  registry->RegisterFunction<NetworkingGetVisibleNetworksFunction>();
+  registry->RegisterFunction<NetworkingRequestConnectFunction>();
+  registry->RegisterFunction<NetworkingRequestDisconnectFunction>();
+}
+
+NetworkingPrivateAPI::~NetworkingPrivateAPI() {
+}
+
+void NetworkingPrivateAPI::Shutdown() {
+}
+
+// static
+NetworkingPrivateAPI* NetworkingPrivateAPI::Get(Profile* profile) {
+  return NetworkingPrivateAPIFactory::GetForProfile(profile);
+}
+
+}  // namespace chromeos