Connectivity Diagnostics API: chrome.diagnostics.sendPacket

chrome/browser/extensions/api/diagnostics/diagnostics_api.cc

+// 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/extensions/api/diagnostics/diagnostics_api.h"
+
+#include "chrome/browser/extensions/api/diagnostics/send_ping_packet.h"
+
+namespace SendPacket = extensions::api::diagnostics::SendPacket;
+
+using base::Callback;

[not at google - send to devlin, 2013/06/20 23:32:12]

not used

[Bei Zhang, 2013/06/21 08:31:07]

Done.

+
+namespace {
+
+const char kErrorPingNotImplemented[] = "Not implemented.";
+const char kErrorPingFailed[] = "Failed to send ping packet.";
+
+}
+
+namespace extensions {
+
+DiagnosticsSendPacketFunction::DiagnosticsSendPacketFunction() {}
+
+DiagnosticsSendPacketFunction::~DiagnosticsSendPacketFunction() {}
+
+bool DiagnosticsSendPacketFunction::Prepare() {
+  parameters_ = SendPacket::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(parameters_.get());
+  return true;
+}
+
+void DiagnosticsSendPacketFunction::AsyncWorkStart() {
+  extensions::SendPingPacket(
+      parameters_->options.ip,
+      parameters_->options.ttl.get(),
+      parameters_->options.timeout.get(),
+      parameters_->options.size.get(),

[not at google - send to devlin, 2013/06/20 23:32:12]

maybe you can just pass parameters->options() into this.

[Bei Zhang, 2013/06/21 08:31:07]

Done.

+      base::Bind(&DiagnosticsSendPacketFunction::OnComplete, this));
+}
+
+bool DiagnosticsSendPacketFunction::Respond() {
+  return error_.empty();
+}
+
+void DiagnosticsSendPacketFunction::OnComplete(
+    SendPingPacketResultCode result_code,
+    const std::string& ip,
+    double latency) {
+

[not at google - send to devlin, 2013/06/20 23:32:12]

no blank line

[Bei Zhang, 2013/06/21 08:31:07]

Done.

+  switch (result_code) {
+    case extensions::SEND_PING_PACKET_OK: {
+      extensions::api::diagnostics::SendPacketResult result;

[not at google - send to devlin, 2013/06/20 23:32:12]

extensions:: unnecessary

also, decide whether to typedef all of these or typedef none of them; more
consistent.

[Bei Zhang, 2013/06/21 08:31:07]

Done.

+      result.ip = ip;
+      result.latency = latency;
+      SetResult(result.ToValue().release());

[not at google - send to devlin, 2013/06/20 23:32:12]

IIRC the "results" are actually a list, the arguments that get passed into the
callback function. SetResult is, confusingly, for single-argument callbacks.

I.e. I believe this needs to be result_ = result.ToValue().Pass();

[Bei Zhang, 2013/06/21 08:31:07]

SetResult creates a new ListValue and appends the given parameter.
result.ToValue() does not return a scoped_ptr<ListValue>. It returns a
scoped_ptr<DictionaryValue> instead.

On 2013/06/20 23:32:12, kalman wrote:

[not at google - send to devlin, 2013/06/21 17:13:24]

Ah I see. Right. What I'm thinking of is SendPacket::Results::Create, which is
designed to be used here. It would be better to use here; more typesafe, easier
to see relationship between that and the function output:

SendPacketResult result;
result.ip = ip;
result.latency = latency;
result_ = SendPacket::Results::Create(result);

[Bei Zhang, 2013/06/21 19:31:44]

Yeah that makes sense. Done.

On 2013/06/21 17:13:24, kalman wrote:

+      break;
+    }
+    case extensions::SEND_PING_PACKET_NOT_IMPLEMENTED:
+      SetError(kErrorPingNotImplemented);
+      break;
+    case extensions::SEND_PING_PACKET_FAILED:
+      SetError(kErrorPingFailed);
+      break;
+  }
+  AsyncWorkCompleted();
+}
+
+}  // namespace extensions