Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17210002:
Connectivity Diagnostics API: chrome.diagnostics.sendPacket (Closed)
|
Created:
7 years, 6 months ago by Bei Zhang Modified:
7 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConnectivity Diagnostics Private API
Proposal:https://docs.google.com/a/chromium.org/document/d/1U_exKvPmT4AXyqFuHgVBbgBZ2RAKNBjk6MwSPcceWXw/edit
Overview
We’d like to have a set of APIs useful for diagnostics tools, and initially, an API that can be used to replicate ping and traceroute functionality.
Use cases
The sendPacket API is used by a diagnostics tool to find problems in the network configuration of an environment and determine latency issues.
BUG=250851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208026
Patch Set 1 #
Total comments: 40
Patch Set 2 : Fixes as per Xiaowen #
Total comments: 3
Patch Set 3 : Use avg instead of time #
Total comments: 10
Patch Set 4 : Change latency type to double #
Total comments: 22
Patch Set 5 : Change the name to diagnosticsPrivate #
Total comments: 17
Patch Set 6 : Fixes #
Total comments: 6
Patch Set 7 : Fix nits #Patch Set 8 : Change namespace name #
Total comments: 20
Patch Set 9 : Fixes #
Total comments: 34
Patch Set 10 : Fixes and rebase #
Total comments: 14
Patch Set 11 : Extract ParseResult #
Total comments: 2
Patch Set 12 : Fix build #Messages
Total messages: 46 (0 generated)
https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/ping_private/ping_private_api.cc (right): https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:29: std::string type = "icmp"; We should return error if the user gives any protocol that's not ICMP here. https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/_permission_features.json:425: }, Add in the whitelist for the connectivity debugger. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.cc File chromeos/dbus/packet_sender.cc (right): https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:29: void PacketSender::Send(std::string ip, std::string type, int ttl, int timeout, Use const std::string&? https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:46: // Parse the result an gives ip and latency "an gives ip and latency" => "and return IP and latency"? https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:52: if (parsed_value->GetAsDictionary(&result) && Need to check whether parsed_value is NULL https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:53: result->GetString("ip", &ip) && result->GetInteger("ttl", &latency)) { It returns something like: { "<ip>" : { ... "time": <int> ... } }, so don't think you can use GetString() like that to get the results.
https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/ping_private/ping_private_api.cc (right): https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:28: std::string ip = parameters_->options.ip; const std::string& ip = parameters_->options.ip; https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:30: long ttl = 1; The generated code for SendPacketOptions.ttl is of type int (scoped_ptr<int> ttl), so this should be int also? https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:34: is the extra empty line needed? https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:35: if (parameters_->options.ttl.get()) .get() isn't needed: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:36: ttl = *parameters_->options.ttl; Maybe this kind of syntax would be easier: int ttl = parameters_->options.ttl ? *parameters_->options.ttl : 1; https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:65: is the extra empty line needed? https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/ping_private/ping_private_api.h (right): https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.h:10: #include "base/memory/ref_counted.h" The code uses scoped_ptr<>, should it include "base/memory/scoped_ptr.h" instead of "base/memory/ref_counted.h"? https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.h:32: void OnComplete(bool succeed, std::string ip, int latency); const std::string& ip? https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/ping_private.idl (right): https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/ping_private.idl:21: is the extra empty line needed? https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h File chromeos/dbus/packet_sender.h (right): https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:17: std::string ip, int latency)> SendCallback; can we use const std::string& ip here? https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:19: virtual ~PacketSender(); is there a reason to use virtual destructor? https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:20: void Send(std::string ip, std::string type, int ttl, int timeout, int size, can we use const std::string& ip here? https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:29: } // namespace chromeos it looks better to insert an empty line here :)
https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/ping_private/ping_private_api.cc (right): https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:31: long timeout = 5000; timeout is specified in seconds, so you probably want to divide this number by 1000.
https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/ping_private/ping_private_api.cc (right): https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:28: std::string ip = parameters_->options.ip; On 2013/06/17 06:19:38, xiaowenx wrote: > const std::string& ip = parameters_->options.ip; Done. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:29: std::string type = "icmp"; Actually we should remove this option for now. We don't really other implementation and it is pretty easy to extend. On 2013/06/17 05:26:41, xiaowenx wrote: > We should return error if the user gives any protocol that's not ICMP here. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:30: long ttl = 1; On 2013/06/17 06:19:38, xiaowenx wrote: > The generated code for SendPacketOptions.ttl is of type int (scoped_ptr<int> > ttl), so this should be int also? Nice catch! It definitely should. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:31: long timeout = 5000; If sub-second timeout could not be specified, then we should not allow user to do it. Stated in docs. On 2013/06/17 07:19:31, xiaowenx wrote: > timeout is specified in seconds, so you probably want to divide this number by > 1000. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:34: On 2013/06/17 06:19:38, xiaowenx wrote: > is the extra empty line needed? Done. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:35: if (parameters_->options.ttl.get()) Hmm, I didn't know this. Yet I've never seen any code tests without the .get(). Maybe without the .get() it is not clear whether we are testing the pointer or the value? On 2013/06/17 06:19:38, xiaowenx wrote: > .get() isn't needed: > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:36: ttl = *parameters_->options.ttl; On 2013/06/17 06:19:38, xiaowenx wrote: > Maybe this kind of syntax would be easier: int ttl = parameters_->options.ttl ? > *parameters_->options.ttl : 1; Done. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.cc:65: On 2013/06/17 06:19:38, xiaowenx wrote: > is the extra empty line needed? Done. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/ping_private/ping_private_api.h (right): https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.h:10: #include "base/memory/ref_counted.h" On 2013/06/17 06:19:38, xiaowenx wrote: > The code uses scoped_ptr<>, should it include "base/memory/scoped_ptr.h" instead > of "base/memory/ref_counted.h"? Done. https://codereview.chromium.org/17210002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/ping_private/ping_private_api.h:32: void OnComplete(bool succeed, std::string ip, int latency); On 2013/06/17 06:19:38, xiaowenx wrote: > const std::string& ip? Done. https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/_permission_features.json:425: }, On 2013/06/17 05:26:41, xiaowenx wrote: > Add in the whitelist for the connectivity debugger. Done. https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/ping_private.idl (right): https://codereview.chromium.org/17210002/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/ping_private.idl:21: On 2013/06/17 06:19:38, xiaowenx wrote: > is the extra empty line needed? Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.cc File chromeos/dbus/packet_sender.cc (right): https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:29: void PacketSender::Send(std::string ip, std::string type, int ttl, int timeout, On 2013/06/17 05:26:41, xiaowenx wrote: > Use const std::string&? Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:46: // Parse the result an gives ip and latency On 2013/06/17 05:26:41, xiaowenx wrote: > "an gives ip and latency" => "and return IP and latency"? Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:52: if (parsed_value->GetAsDictionary(&result) && On 2013/06/17 05:26:41, xiaowenx wrote: > Need to check whether parsed_value is NULL Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.c... chromeos/dbus/packet_sender.cc:53: result->GetString("ip", &ip) && result->GetInteger("ttl", &latency)) { On 2013/06/17 05:26:41, xiaowenx wrote: > It returns something like: { "<ip>" : { ... "time": <int> ... } }, so don't > think you can use GetString() like that to get the results. Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h File chromeos/dbus/packet_sender.h (right): https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:17: std::string ip, int latency)> SendCallback; On 2013/06/17 06:19:38, xiaowenx wrote: > can we use const std::string& ip here? Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:17: std::string ip, int latency)> SendCallback; On 2013/06/17 06:19:38, xiaowenx wrote: > can we use const std::string& ip here? Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:20: void Send(std::string ip, std::string type, int ttl, int timeout, int size, On 2013/06/17 06:19:38, xiaowenx wrote: > can we use const std::string& ip here? Done. https://codereview.chromium.org/17210002/diff/1/chromeos/dbus/packet_sender.h... chromeos/dbus/packet_sender.h:29: } // namespace chromeos On 2013/06/17 06:19:38, xiaowenx wrote: > it looks better to insert an empty line here :) Done.
https://codereview.chromium.org/17210002/diff/21001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.h (right): https://codereview.chromium.org/17210002/diff/21001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:9: #include "base/values.h" this include seems unnecessary? https://codereview.chromium.org/17210002/diff/21001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:23: // If omitted If omitted?
https://codereview.chromium.org/17210002/diff/21001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.h (right): https://codereview.chromium.org/17210002/diff/21001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:9: #include "base/values.h" On 2013/06/17 08:00:00, xiaowenx wrote: > this include seems unnecessary? Done.
https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.h (right): https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:7: #include <string>
https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.cc (right): https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:35: std::map<std::string, std::string> config; Need to add count=1 as parameter to TestICMPWithOptions.
https://codereview.chromium.org/17210002/diff/28001/chrome/common/extensions/... File chrome/common/extensions/api/ping_private.idl (right): https://codereview.chromium.org/17210002/diff/28001/chrome/common/extensions/... chrome/common/extensions/api/ping_private.idl:19: long latency; This is returned as type double.
https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.cc (right): https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:62: if (info->GetInteger("avg", &latency)) { double latency; info->GetDouble(...) https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.h (right): https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:17: int latency)> SendCallback; double latency
https://codereview.chromium.org/17210002/diff/28001/chrome/common/extensions/... File chrome/common/extensions/api/ping_private.idl (right): https://codereview.chromium.org/17210002/diff/28001/chrome/common/extensions/... chrome/common/extensions/api/ping_private.idl:19: long latency; On 2013/06/17 08:33:08, xiaowenx wrote: > This is returned as type double. Done. https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.cc (right): https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:35: std::map<std::string, std::string> config; On 2013/06/17 08:29:34, xiaowenx wrote: > Need to add count=1 as parameter to TestICMPWithOptions. Done. https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:62: if (info->GetInteger("avg", &latency)) { On 2013/06/17 08:41:32, xiaowenx wrote: > double latency; info->GetDouble(...) Done. https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.h (right): https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:7: On 2013/06/17 08:19:55, xiaowenx wrote: > #include <string> Done. https://codereview.chromium.org/17210002/diff/28001/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:17: int latency)> SendCallback; On 2013/06/17 08:41:32, xiaowenx wrote: > double latency Done.
lgtm
What is this?
On 2013/06/17 16:10:51, kalman wrote: > What is this? API proposal here: https://docs.google.com/a/chromium.org/document/d/1U_exKvPmT4AXyqFuHgVBbgBZ2R... and signed up for API review for this week. Happy to answer any more questions ahead of it though.
On 2013/06/17 16:10:51, kalman wrote: > What is this? This needs a bug id.
On 2013/06/17 16:42:53, stevenjb (chromium) wrote: > On 2013/06/17 16:10:51, kalman wrote: > > What is this? > > This needs a bug id. OK, I just filed this: crbug.com/250851. ikarienator, want to add this to the review description?
Hi Steven, Just added a bug Id. Please review. Thanks, Bei On 2013/06/17 16:42:53, stevenjb (chromium) wrote: > On 2013/06/17 16:10:51, kalman wrote: > > What is this? > > This needs a bug id.
https://chromiumcodereview.appspot.com/17210002/diff/23003/chrome/browser/ext... File chrome/browser/extensions/api/ping_private/ping_private_api.h (right): https://chromiumcodereview.appspot.com/17210002/diff/23003/chrome/browser/ext... chrome/browser/extensions/api/ping_private/ping_private_api.h:24: nit: Use a comment to note which class these are overriding (less critical in a small class like this but still helpful): // AsyncApiFunction https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... File chromeos/dbus/icmp_packet_sender.cc (right): https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.cc:27: return Singleton<ICMPPacketSender>::get(); Avoid using Singleton<> in classes with an explicit dependency like DBusThreadmanager. This can probably be owned by DBusThreadManager, or maybe another appropriate class. https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.cc:31: int* ttl, int* timeout, int* size, one param per line https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.cc:45: base::Unretained(this), Avoid Untretained with DBus calls. It looks like OnSend can be a non-member function defined in an anonymous namespace which avoids the issue entirely. Otherwise a WeakPtrFactory should be used. https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.cc:50: bool succeeded, const std::string& status) { One param per line. https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.cc:51: if (succeeded) { Early exit: if (!succeeded) { callback.Run(false, "", 0); return; } https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... File chromeos/dbus/icmp_packet_sender.h (right): https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.h:24: // Pass NULL for omitted argument. s/Send/Sends/ s/Pass/Passes/ s/argument/arguments/ https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.h:25: void Send(const std::string& ip, int* ttl, int* timeout, int* size, Use const int* for optional input parameters. https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.h:26: const SendCallback& callback); One parameter per line https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.h:28: static ICMPPacketSender* GetInstance(); nit: blank line https://chromiumcodereview.appspot.com/17210002/diff/23003/chromeos/dbus/icmp... chromeos/dbus/icmp_packet_sender.h:31: bool succeeded, const std::string& reply); One param per line (but see note about making this a non-member)
https://codereview.chromium.org/17210002/diff/23003/chrome/browser/extensions... File chrome/browser/extensions/api/ping_private/ping_private_api.h (right): https://codereview.chromium.org/17210002/diff/23003/chrome/browser/extensions... chrome/browser/extensions/api/ping_private/ping_private_api.h:24: On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > nit: Use a comment to note which class these are overriding (less critical in a > small class like this but still helpful): > // AsyncApiFunction Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.cc (right): https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:27: return Singleton<ICMPPacketSender>::get(); On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > Avoid using Singleton<> in classes with an explicit dependency like > DBusThreadmanager. This can probably be owned by DBusThreadManager, or maybe > another appropriate class. Done. Moved to the same directory containing the api implementation. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:31: int* ttl, int* timeout, int* size, On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > one param per line Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:45: base::Unretained(this), On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > Avoid Untretained with DBus calls. It looks like OnSend can be a non-member > function defined in an anonymous namespace which avoids the issue entirely. > Otherwise a WeakPtrFactory should be used. Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:50: bool succeeded, const std::string& status) { On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > One param per line. Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.cc:51: if (succeeded) { On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > Early exit: > if (!succeeded) { > callback.Run(false, "", 0); > return; > } Done. Changed the signature of the callback to have an error message. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... File chromeos/dbus/icmp_packet_sender.h (right): https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:24: // Pass NULL for omitted argument. On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > s/Send/Sends/ > s/Pass/Passes/ > s/argument/arguments/ Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:25: void Send(const std::string& ip, int* ttl, int* timeout, int* size, On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > Use const int* for optional input parameters. Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:26: const SendCallback& callback); On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > One parameter per line Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:28: static ICMPPacketSender* GetInstance(); On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > nit: blank line Done. https://codereview.chromium.org/17210002/diff/23003/chromeos/dbus/icmp_packet... chromeos/dbus/icmp_packet_sender.h:31: bool succeeded, const std::string& reply); On 2013/06/17 17:24:24, stevenjb (chromium) wrote: > One param per line (but see note about making this a non-member) Done.
https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:40: std::string ip = iterator.key(); const std::string& ip = iterator.key()? https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:51: return; Unnecessary return? The other way to do this is to continue checking for error, and then do callback.Run(<success case>) at the very bottom of this method.
https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc (right): https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc:26: SendPingPacket( qualify this with extensions:: to make it clear that this is a namespaced function and not a member. https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h:16: // Otherwise the |ip| and |latency| arguments are not defined. This is an odd API (and the comment is kind of confusing). I'd prefer to use a result code (i.e. an enum 'result' instead of string 'error') here for two reasons: 1. The code will be more clear. 2. Error messages should be define as consts in the api .cc file (see other examples of SetError calls). https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:51: return; On 2013/06/18 19:07:37, xiaowenx wrote: > Unnecessary return? The other way to do this is to continue checking for error, > and then do callback.Run(<success case>) at the very bottom of this method. +1. I'd suggest: const base::DictionaryValue* info; double latency; if (!iterator.value().GetAsDictionary(&info) || !info->GetDouble("avg", &latency)) { callback.Run("Malformed reply from the backend.", "", 0.0); return; } // Success callback.Run("", ip, latency); } (Note, you won't need a "Success" comment if you switch to using a result code instead of using an empty error string to indicate success) https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:63: chromeos::DBusThreadManager::Get()->GetDebugDaemonClient(); no need for local used just once further down https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:65: config["count"] = "1"; "count" and other keys should be consts defined at the top of the file (in the anon namespace) https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:66: if (ttl) config["ttl"] = base::IntToString(*ttl); 2 lines here and below https://codereview.chromium.org/17210002/diff/18002/chrome/chrome_browser_ext... File chrome/chrome_browser_extensions.gypi (right): https://codereview.chromium.org/17210002/diff/18002/chrome/chrome_browser_ext... chrome/chrome_browser_extensions.gypi:795: 'browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc', This should exclude send_ping_packet.cc, shouldn't it? Also, alphabetize.
Thank you so much for the review! Very helpful! https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc (right): https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc:26: SendPingPacket( On 2013/06/18 19:19:28, stevenjb (chromium) wrote: > qualify this with extensions:: to make it clear that this is a namespaced > function and not a member. Done. https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h:16: // Otherwise the |ip| and |latency| arguments are not defined. On 2013/06/18 19:19:28, stevenjb (chromium) wrote: > This is an odd API (and the comment is kind of confusing). I'd prefer to use a > result code (i.e. an enum 'result' instead of string 'error') here for two > reasons: > 1. The code will be more clear. > 2. Error messages should be define as consts in the api .cc file (see other > examples of SetError calls). > Done. https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:40: std::string ip = iterator.key(); On 2013/06/18 19:07:37, xiaowenx wrote: > const std::string& ip = iterator.key()? Done. https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:51: return; On 2013/06/18 19:19:28, stevenjb (chromium) wrote: > On 2013/06/18 19:07:37, xiaowenx wrote: > > Unnecessary return? The other way to do this is to continue checking for > error, > > and then do callback.Run(<success case>) at the very bottom of this method. > +1. I'd suggest: > > const base::DictionaryValue* info; > double latency; > if (!iterator.value().GetAsDictionary(&info) || > !info->GetDouble("avg", &latency)) { > callback.Run("Malformed reply from the backend.", "", 0.0); > return; > } > // Success > callback.Run("", ip, latency); > } > > (Note, you won't need a "Success" comment if you switch to using a result code > instead of using an empty error string to indicate success) > Done. https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:63: chromeos::DBusThreadManager::Get()->GetDebugDaemonClient(); On 2013/06/18 19:19:28, stevenjb (chromium) wrote: > no need for local used just once further down Actually, I should test whether they are available. https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:65: config["count"] = "1"; On 2013/06/18 19:19:28, stevenjb (chromium) wrote: > "count" and other keys should be consts defined at the top of the file (in the > anon namespace) Done. https://codereview.chromium.org/17210002/diff/18002/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:66: if (ttl) config["ttl"] = base::IntToString(*ttl); On 2013/06/18 19:19:28, stevenjb (chromium) wrote: > 2 lines here and below Done. https://codereview.chromium.org/17210002/diff/18002/chrome/chrome_browser_ext... File chrome/chrome_browser_extensions.gypi (right): https://codereview.chromium.org/17210002/diff/18002/chrome/chrome_browser_ext... chrome/chrome_browser_extensions.gypi:795: 'browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc', On 2013/06/18 19:19:28, stevenjb (chromium) wrote: > This should exclude send_ping_packet.cc, shouldn't it? Also, alphabetize. Done.
Close! https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc (right): https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc:56: case extensions::SEND_PING_PACKET_NOT_IMPLEMENTED: indent case 2 spaces past switch, following lines 2 spaces past case. That said, either handle the 'OK' case below or just use else if(); the NOTREACHED below is confusing. https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h:14: enum SendPingPacketErrorCode { This should really be SendPingPacketResultCode since OK is not an error. Variables should be named 'result_code' to match. https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:71: = chromeos::DBusThreadManager::Get(); nit: = on previous line
https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc (right): https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/diagnostics_private_api.cc:56: case extensions::SEND_PING_PACKET_NOT_IMPLEMENTED: On 2013/06/18 22:14:51, stevenjb (chromium) wrote: > indent case 2 spaces past switch, following lines 2 spaces past case. > That said, either handle the 'OK' case below or just use else if(); the > NOTREACHED below is confusing. Done. https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet.h:14: enum SendPingPacketErrorCode { On 2013/06/18 22:14:51, stevenjb (chromium) wrote: > This should really be SendPingPacketResultCode since OK is not an error. > Variables should be named 'result_code' to match. Done. https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/69001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics_private/send_ping_packet_chromeos.cc:71: = chromeos::DBusThreadManager::Get(); On 2013/06/18 22:14:51, stevenjb (chromium) wrote: > nit: = on previous line Done. Sorry for a nits like this...
LGTM
Hi all, After the API review meeting, we agreed on the new name. Please review. Thanks, Bei
https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics/diagnostics_api.cc (right): https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:64: } Do you want to have a default case that also does SetError(kErrorPingNotImplemented)? https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/_permission_features.json:149: }], White-spacing on this looks off. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/permissions/api_permission.h (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/permissions/api_permission.h:62: kDiagnosticsPrivate, Should this be changed to kDiagnostics instead? https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/permissions/chrome_api_permissions.cc:152: { APIPermission::kDiagnosticsPrivate, "diagnosticsPrivate", diagnostics instead of diagnosticsPrivate?
lgtm with a couple of nits you should fix before checking in, and an optional suggestion. https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics/diagnostics_api.cc (right): https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:16: const char kErrorPingFailed[] = "Failed to send ping packet."; nit: I think we usually don't end the developer-readable error messages with a '.' character. https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:12: namespace extensions { optional suggestion: It might make more sense to have this namespace be "extensions::api::diagnostics", and then perhaps rename SendPingPacket to "SendPacketImpl" Alternatively, consider leaving in extensions namespace but renaming SendPingPacket to "DiagnosticsSendPacket". https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:32: const int* size, nit: you should just pass the integer values here by value, not by const pointer. (Usually we only use pointer arguments for objects/structs or out-params). https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/_permission_features.json:149: }], On 2013/06/20 19:42:44, xiaowenx wrote: > White-spacing on this looks off. Agreed - your lines 139-149 should be indented similarly to lines 40-52 in this file. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/api/diagnostics.idl (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/diagnostics.idl:14: long? size; It would be good mention something in the comments for these 3 optional parameters about what default values the platform will use if the developer doesn't specify explicit values (if it's platform specific and not always the same, you can just say that too). https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/diagnostics.idl:18: DOMString ip; nit: It's not clear what the meaning of this field is - can you include a comment about it's meaning, and under what circumstances it will differ from the value of the ip passed in the SendPacketOptions dictionary? https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/diagnostics.idl:19: double latency; nit: Can you add a comment here about the units of latency here? (In addition, you could rename the field to be self-documenting, e.g. "latencyMs" if this is in milliseconds).
https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.cc (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:11: using base::Callback; not used https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:37: parameters_->options.size.get(), maybe you can just pass parameters->options() into this. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:49: no blank line https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:52: extensions::api::diagnostics::SendPacketResult result; extensions:: unnecessary also, decide whether to typedef all of these or typedef none of them; more consistent. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:55: SetResult(result.ToValue().release()); 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(); https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.h (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:17: class DiagnosticsSendPacketFunction : public AsyncApiFunction { What thread does this stuff happen on? Is it the UI thread? In which case using just an AsyncExtensionFunction would be simpler, you only need to implement RunImpl (and for that matter I actually find AsyncApiFunction quite a confusingly indirect class). https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:13: this should be in a second nested namespace like send_ping_packet, and the names updated accordingly. I am somewhat confused why it isn't a class, though. It seems idiomatically a class, and I've seen that pattern many times for cross-platform things. I.e. the class has a static method like Create(). https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:27: // Passes NULL for omitted arguments. You need to document that callback can be called synchronously - or just always make sure to reply on MessageLoop::current(). https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:30: const int* ttl, as I said in the other file, it would be cleaner to just pass the whole options into here. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:33: base::DictionaryValue* result; initialize pointers to NULL https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:34: if (!parsed_value.get() || !parsed_value->GetAsDictionary(&result)) { just check parsed_value, get() not necessary. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:43: return; this check should really be that size() == 1 since otherwise the "first entry" is undefined. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:55: callback.Run(SEND_PING_PACKET_FAILED, "", 0.0); For all of these failures, is there something more descriptive you can send back to the extension? https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:72: if (!dbus_thread_manager) { when can this happen? https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:79: if (!debugd_client) { when can this happen?
https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics/diagnostics_api.cc (right): https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:16: const char kErrorPingFailed[] = "Failed to send ping packet."; On 2013/06/20 22:51:29, Antony Sargent wrote: > nit: I think we usually don't end the developer-readable error messages with a > '.' character. Done. https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:64: } default case for enums are not recommended as it makes it hard to locate a missing case. We have an automatic tool to check if all the cases are covered. On 2013/06/20 19:42:44, xiaowenx wrote: > Do you want to have a default case that also does > SetError(kErrorPingNotImplemented)? https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... File chrome/browser/extensions/api/diagnostics/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/92001/chrome/browser/extensions... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:32: const int* size, On 2013/06/20 22:51:29, Antony Sargent wrote: > nit: you should just pass the integer values here by value, not by const > pointer. (Usually we only use pointer arguments for objects/structs or > out-params). Done. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/_permission_features.json:149: }], On 2013/06/20 19:42:44, xiaowenx wrote: > White-spacing on this looks off. Done. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/api/diagnostics.idl (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/diagnostics.idl:14: long? size; On 2013/06/20 22:51:29, Antony Sargent wrote: > It would be good mention something in the comments for these 3 optional > parameters about what default values the platform will use if the developer > doesn't specify explicit values (if it's platform specific and not always the > same, you can just say that too). Done. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/diagnostics.idl:18: DOMString ip; On 2013/06/20 22:51:29, Antony Sargent wrote: > nit: It's not clear what the meaning of this field is - can you include a > comment about it's meaning, and under what circumstances it will differ from the > value of the ip passed in the SendPacketOptions dictionary? Done. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/api/diagnostics.idl:19: double latency; On 2013/06/20 22:51:29, Antony Sargent wrote: > nit: Can you add a comment here about the units of latency here? (In addition, > you could rename the field to be self-documenting, e.g. "latencyMs" if this is > in milliseconds). Done. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/permissions/api_permission.h (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/permissions/api_permission.h:62: kDiagnosticsPrivate, On 2013/06/20 19:42:44, xiaowenx wrote: > Should this be changed to kDiagnostics instead? Done. https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/17210002/diff/92001/chrome/common/extensions/... chrome/common/extensions/permissions/chrome_api_permissions.cc:152: { APIPermission::kDiagnosticsPrivate, "diagnosticsPrivate", On 2013/06/20 19:42:44, xiaowenx wrote: > diagnostics instead of diagnosticsPrivate? Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.cc (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:11: using base::Callback; On 2013/06/20 23:32:12, kalman wrote: > not used Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:37: parameters_->options.size.get(), On 2013/06/20 23:32:12, kalman wrote: > maybe you can just pass parameters->options() into this. Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:49: On 2013/06/20 23:32:12, kalman wrote: > no blank line Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:52: extensions::api::diagnostics::SendPacketResult result; On 2013/06/20 23:32:12, kalman wrote: > extensions:: unnecessary > > also, decide whether to typedef all of these or typedef none of them; more > consistent. Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:55: SetResult(result.ToValue().release()); 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: > 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(); https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.h (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:17: class DiagnosticsSendPacketFunction : public AsyncApiFunction { AsyncApiFunction works on IO thread. On 2013/06/20 23:32:12, kalman wrote: > What thread does this stuff happen on? Is it the UI thread? In which case using > just an AsyncExtensionFunction would be simpler, you only need to implement > RunImpl (and for that matter I actually find AsyncApiFunction quite a > confusingly indirect class). https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/send_ping_packet.h (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:13: On 2013/06/20 23:32:12, kalman wrote: > this should be in a second nested namespace like send_ping_packet, and the names > updated accordingly. > > I am somewhat confused why it isn't a class, though. It seems idiomatically a > class, and I've seen that pattern many times for cross-platform things. I.e. the > class has a static method like Create(). Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:27: // Passes NULL for omitted arguments. I removed this method. Now it's a private method inside extensions::DiagnosticsSendPacketFunction On 2013/06/20 23:32:12, kalman wrote: > You need to document that callback can be called synchronously - or just always > make sure to reply on MessageLoop::current(). https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet.h:30: const int* ttl, On 2013/06/20 23:32:12, kalman wrote: > as I said in the other file, it would be cleaner to just pass the whole options > into here. Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:33: base::DictionaryValue* result; On 2013/06/20 23:32:12, kalman wrote: > initialize pointers to NULL Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:34: if (!parsed_value.get() || !parsed_value->GetAsDictionary(&result)) { On 2013/06/20 23:32:12, kalman wrote: > just check parsed_value, get() not necessary. Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:43: return; On 2013/06/20 23:32:12, kalman wrote: > this check should really be that size() == 1 since otherwise the "first entry" > is undefined. Done. https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:55: callback.Run(SEND_PING_PACKET_FAILED, "", 0.0); Our chromeos backend currently does not distinguish the differences between routing error and timeout. On 2013/06/20 23:32:12, kalman wrote: > For all of these failures, is there something more descriptive you can send back > to the extension? https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:72: if (!dbus_thread_manager) { For the current code, it is protected by a CHECK. Removing the test. On 2013/06/20 23:32:12, kalman wrote: > when can this happen? https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/send_ping_packet_chromeos.cc:79: if (!debugd_client) { It never happens for the current code. On 2013/06/20 23:32:12, kalman wrote: > when can this happen?
https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.cc (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:55: SetResult(result.ToValue().release()); On 2013/06/21 08:31:07, Bei Zhang wrote: > 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: > > 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(); > 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); https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.h (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:17: class DiagnosticsSendPacketFunction : public AsyncApiFunction { On 2013/06/21 08:31:07, Bei Zhang wrote: > AsyncApiFunction works on IO thread. > > On 2013/06/20 23:32:12, kalman wrote: > > What thread does this stuff happen on? Is it the UI thread? In which case > using > > just an AsyncExtensionFunction would be simpler, you only need to implement > > RunImpl (and for that matter I actually find AsyncApiFunction quite a > > confusingly indirect class). > Ok. Could you add some DCHECK(CurrentyOn(IO))s around the place then. That isn't clear. https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.h (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:20: enum SendPacketResultCode { is this only used internally? if so it can be private https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:41: virtual void AsyncWorkStart() OVERRIDE; comment here that this is implemented differently for chromeos and other platforms. https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:28: const std::string& ip, double latency)>& callback, TestICMPCallback? https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:56: } while (false); I hope there is a way to factor this without emulating goto.
https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:56: } while (false); On 2013/06/21 17:13:24, kalman wrote: > I hope there is a way to factor this without emulating goto. I suppose another way is to just write the goto instead of obfuscating it inside a do/while. One of the few places goto is useful is for error handling. You'd still need the scope though, since goto can't jump into the middle of a variable's lifetime.
https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:56: } while (false); On 2013/06/21 17:33:50, Jeffrey Yasskin wrote: > On 2013/06/21 17:13:24, kalman wrote: > > I hope there is a way to factor this without emulating goto. > > I suppose another way is to just write the goto instead of obfuscating it inside > a do/while. One of the few places goto is useful is for error handling. You'd > still need the scope though, since goto can't jump into the middle of a > variable's lifetime. I would prefer to factor it into a function that returns success/failure, on failure do the SEND_PACKET_FAILED. Incidentally - what does it mean for the response from that library to be malformed? I am wondering whether these should CHECK. But, whatever.
https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:29: bool succeeded, const std::string& status) { Each parameter on its own line https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:56: } while (false); On 2013/06/21 17:40:44, kalman wrote: > On 2013/06/21 17:33:50, Jeffrey Yasskin wrote: > > On 2013/06/21 17:13:24, kalman wrote: > > > I hope there is a way to factor this without emulating goto. > > > > I suppose another way is to just write the goto instead of obfuscating it > inside > > a do/while. One of the few places goto is useful is for error handling. You'd > > still need the scope though, since goto can't jump into the middle of a > > variable's lifetime. > > I would prefer to factor it into a function that returns success/failure, on > failure do the SEND_PACKET_FAILED. > > Incidentally - what does it mean for the response from that library to be > malformed? I am wondering whether these should CHECK. But, whatever. +1, this should be broken up into a subfunction that sets ip and latency and returns true, or returns false. Then the callback can be run with OK or FAILED from here.
https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.cc (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.cc:55: SetResult(result.ToValue().release()); Yeah that makes sense. Done. On 2013/06/21 17:13:24, kalman wrote: > On 2013/06/21 08:31:07, Bei Zhang wrote: > > 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: > > > 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(); > > > > 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); https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.h (right): https://codereview.chromium.org/17210002/diff/100001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:17: class DiagnosticsSendPacketFunction : public AsyncApiFunction { Actually all the apps APIs are by default works on IO thread. More specifically the pattern we are using is that PrePrepare and Prepare happens on UI thread and AsyncWorkStart starts on IO thread. AsyncWorkComplete can be invoked on any thread. Following this pattern we probably don't need to DCHECK the thread here. On 2013/06/21 17:13:24, kalman wrote: > On 2013/06/21 08:31:07, Bei Zhang wrote: > > AsyncApiFunction works on IO thread. > > > > On 2013/06/20 23:32:12, kalman wrote: > > > What thread does this stuff happen on? Is it the UI thread? In which case > > using > > > just an AsyncExtensionFunction would be simpler, you only need to implement > > > RunImpl (and for that matter I actually find AsyncApiFunction quite a > > > confusingly indirect class). > > > > Ok. Could you add some DCHECK(CurrentyOn(IO))s around the place then. That isn't > clear. https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api.h (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:20: enum SendPacketResultCode { A static function in diagnostics_api_chromeos.cc is using this enum. I'm trying to avoid making it a friend function because it is only used on that platform. On 2013/06/21 17:13:24, kalman wrote: > is this only used internally? if so it can be private https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api.h:41: virtual void AsyncWorkStart() OVERRIDE; On 2013/06/21 17:13:24, kalman wrote: > comment here that this is implemented differently for chromeos and other > platforms. Done. https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:28: const std::string& ip, double latency)>& callback, On 2013/06/21 17:13:24, kalman wrote: > TestICMPCallback? Done. I prefer SendPacketCallback because it's not directly called from DebugDaemonClient. https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:29: bool succeeded, const std::string& status) { On 2013/06/21 18:35:21, stevenjb (chromium) wrote: > Each parameter on its own line Done. https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:56: } while (false); This idea is great! We are actually using the do-while-false pattern on some other places. I probably should clean them up. On 2013/06/21 18:35:21, stevenjb (chromium) wrote: > On 2013/06/21 17:40:44, kalman wrote: > > On 2013/06/21 17:33:50, Jeffrey Yasskin wrote: > > > On 2013/06/21 17:13:24, kalman wrote: > > > > I hope there is a way to factor this without emulating goto. > > > > > > I suppose another way is to just write the goto instead of obfuscating it > > inside > > > a do/while. One of the few places goto is useful is for error handling. > You'd > > > still need the scope though, since goto can't jump into the middle of a > > > variable's lifetime. > > > > I would prefer to factor it into a function that returns success/failure, on > > failure do the SEND_PACKET_FAILED. > > > > Incidentally - what does it mean for the response from that library to be > > malformed? I am wondering whether these should CHECK. But, whatever. > +1, this should be broken up into a subfunction that sets ip and latency and > returns true, or returns false. Then the callback can be run with OK or FAILED > from here. > >
lgtm https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/112005/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:28: const std::string& ip, double latency)>& callback, On 2013/06/21 19:31:44, Bei Zhang wrote: > On 2013/06/21 17:13:24, kalman wrote: > > TestICMPCallback? > > Done. I prefer SendPacketCallback because it's not directly called from > DebugDaemonClient. Well there was already a TestICMPCallback in that other file. https://codereview.chromium.org/17210002/diff/168001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/168001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:68: latency); It would be simpler to let ParseResult send the OK response, then you wouldn't need to pass the ip and the latency out. Call it something else though. Failure response would still be called from here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/17210002/168001
Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/extensions/extension_function_histogram_value.h
Hunk #1 FAILED at 552.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej
Patch: chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
6b1a5d4dede29bc0242b1cc313ab76a9a5e8593a..da3a0f2c13d015be44a6fc96f26e0fe5b0cd6cec
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -552,6 +552,7 @@ enum HistogramValue {
FEEDBACKPRIVATE_GETUSEREMAIL,
FEEDBACKPRIVATE_GETSYSTEMINFORMATION,
FEEDBACKPRIVATE_SENDFEEDBACK,
+ DIAGNOSTICS_SENDPACKET,
ENUM_BOUNDARY // Last entry: Add new entries above.
};
Thank you so much for the review! https://codereview.chromium.org/17210002/diff/168001/chrome/browser/extension... File chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc (right): https://codereview.chromium.org/17210002/diff/168001/chrome/browser/extension... chrome/browser/extensions/api/diagnostics/diagnostics_api_chromeos.cc:68: latency); I think it'll be weird if you pass in a callback and relies on its also being called outside the function. On 2013/06/22 00:14:33, kalman wrote: > It would be simpler to let ParseResult send the OK response, then you wouldn't > need to pass the ip and the latency out. Call it something else though. Failure > response would still be called from here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/17210002/13002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/17210002/186001
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/17210002/186001
Message was sent while issue was closed.
Change committed as 208026 |
