Replace chrome://network implementation with networkConfig API

chrome/browser/ui/webui/chromeos/network_config_message_handler.cc

+// Copyright 2014 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/ui/webui/chromeos/network_config_message_handler.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/values.h"
+#include "chromeos/network/favorite_state.h"
+#include "chromeos/network/managed_network_configuration_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_util.h"
+#include "chromeos/network/onc/onc_signature.h"
+#include "chromeos/network/onc/onc_utils.h"
+#include "chromeos/network/shill_property_util.h"
+#include "components/onc/onc_constants.h"
+#include "content/public/browser/web_ui.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {
+
+bool GetServicePathFromGuid(const std::string& guid,
+                            std::string* service_path) {
+  const FavoriteState* network =
+      NetworkHandler::Get()->network_state_handler()->GetFavoriteStateFromGuid(
+          guid);
+  if (!network)
+    return false;
+  *service_path = network->path();
+  return true;
+}
+
+}  // namespace
+
+NetworkConfigMessageHandler::NetworkConfigMessageHandler() {
+}
+
+NetworkConfigMessageHandler::~NetworkConfigMessageHandler() {
+}
+
+void NetworkConfigMessageHandler::RegisterMessages() {
+  web_ui()->RegisterMessageCallback(
+      "networkConfig.getNetworks",
+      base::Bind(&NetworkConfigMessageHandler::GetNetworks,
+                 base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "networkConfig.getProperties",
+      base::Bind(&NetworkConfigMessageHandler::GetProperties,
+                 base::Unretained(this)));
+}
+
+void NetworkConfigMessageHandler::GetNetworks(
+    const base::ListValue* arg_list) const {
+  int callback_id;
+  const base::DictionaryValue* filter;
+  if (!arg_list->GetInteger(0, &callback_id) ||
+      !arg_list->GetDictionary(1, &filter)) {
+    NOTREACHED();

[pneubeck (no reviews), 2014/05/22 16:24:20]

at least return; but as the "rule" is to not handle DCHECKs or NOTREACHEDs, and
crashing the browser is not necessary, these should be LOG(ERROR)s.

[stevenjb, 2014/05/23 21:50:59]

This represents a (JS) coding error, so should crash if it happens. It's simpler
than assigning each result to a bool and DCHECK(success) on each.

[pneubeck (no reviews), 2014/05/24 13:38:53]

IMO not acceptable.
NOTREACHED will just succeed on release builds, in which case this would
continue with uninitialized callback_id&filter.
Better to be on the safe side and return or crash in this case.

what I saw in other handlers is 'NOTREACHED(); return;' (although that's not
following the dev.chromiumg style) or 'CHECK'

[stevenjb, 2014/05/27 15:58:35]

'filter' should be initialized to NULL here so that we crash in Release.

+  }
+  std::string type = ::onc::network_type::kAllTypes;
+  bool visible_only = false;
+  bool configured_only = false;
+  int limit = 1000;
+  filter->GetString("type", &type);
+  filter->GetBoolean("visible", &visible_only);
+  filter->GetBoolean("configured", &configured_only);
+  filter->GetInteger("limit", &limit);
+
+  base::ListValue return_arg_list;
+  return_arg_list.AppendInteger(callback_id);
+
+  NetworkTypePattern pattern = onc::NetworkTypePatternFromOncType(type);

[pneubeck (no reviews), 2014/05/22 16:24:20]

nit: move to the other filter options above

[stevenjb, 2014/05/23 21:50:59]

Done.

+  scoped_ptr<base::ListValue> network_properties_list =
+      chromeos::network_util::TranslateNetworkListToONC(
+          pattern, configured_only, visible_only, limit,
+          true /* debugging_properties */);
+
+  return_arg_list.Append(network_properties_list.release());
+
+  InvokeCallback(return_arg_list);
+}
+
+void NetworkConfigMessageHandler::GetProperties(
+    const base::ListValue* arg_list) const {
+  int callback_id;
+  std::string guid;
+  if (!arg_list->GetInteger(0, &callback_id) ||

[xiyuan, 2014/05/21 21:22:28]

nit: I would just CHECK, i.e. CHECK(arg_list->GetInteger(0, &callback_id));

[stevenjb, 2014/05/23 21:50:59]

1) It's non-fatal but should never happen, so DCHECK (i.e. NOTREACHED) should be
sufficient.
2) I really don't like embedding necessary code inside a macro.

+      !arg_list->GetString(1, &guid)) {
+    NOTREACHED();
+  }
+  std::string service_path;
+  if (!GetServicePathFromGuid(guid, &service_path)) {
+    scoped_ptr<base::DictionaryValue> error_data;
+    ErrorCallback(callback_id, "Error.InvalidNetworkGuid", error_data.Pass());
+    return;
+  }
+  NetworkHandler::Get()->managed_network_configuration_handler()->GetProperties(
+      service_path,
+      base::Bind(&NetworkConfigMessageHandler::GetPropertiesSuccess,
+                 base::Unretained(this), callback_id),

[xiyuan, 2014/05/21 21:22:28]

Are we sure this class would not go away when GetProperties invokes the
callback? If not, we should use a WeakPtr (e.g. via WeakPtrFactory) here.

[stevenjb, 2014/05/23 21:50:59]

Yes, yes we should. I must have copy/pasted this. Fixed.

+      base::Bind(&NetworkConfigMessageHandler::ErrorCallback,
+                 base::Unretained(this), callback_id));
+}
+
+void NetworkConfigMessageHandler::GetPropertiesSuccess(
+    int callback_id,
+    const std::string& service_path,
+    const base::DictionaryValue& dictionary) const {
+  base::ListValue return_arg_list;
+  return_arg_list.AppendInteger(callback_id);
+
+  base::DictionaryValue* network_properties = dictionary.DeepCopy();
+  network_properties->SetStringWithoutPathExpansion(
+      ::onc::network_config::kGUID, service_path);
+  return_arg_list.Append(network_properties);
+  InvokeCallback(return_arg_list);
+}
+
+void NetworkConfigMessageHandler::InvokeCallback(
+    const base::ListValue& arg_list) const {
+  web_ui()->CallJavascriptFunction(
+      "networkConfig.chromeCallbackSuccess", arg_list);
+}
+
+void NetworkConfigMessageHandler::ErrorCallback(
+    int callback_id,
+    const std::string& error_name,
+    scoped_ptr<base::DictionaryValue> error_data) const {
+  LOG(ERROR) << "NetworkConfigMessageHandler Error: " << error_name;
+  base::ListValue arg_list;
+  arg_list.AppendInteger(callback_id);
+  arg_list.AppendString(error_name);
+  web_ui()->CallJavascriptFunction(
+      "networkConfig.chromeCallbackError", arg_list);
+}
+
+}  // namespace chromeos