Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
996013003:
privetd: Expose dbus API (Closed)
|
Created:
5 years, 9 months ago by dtapuska Modified:
5 years, 8 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionprivetd: Expose dbus API
privetd is a chromeos service that handles device bootstrapping
and setup.
Expose an API to provide events from the privetd dbus.
BUG=467018
Committed: https://crrev.com/ab7987e2b286aead19236e89718f8cd97b242ec1
Cr-Commit-Position: refs/heads/master@{#321621}
Patch Set 1 #
Total comments: 44
Patch Set 2 : Replace Property<> usage with a class deriving from PropertyBase and add unit tests #
Total comments: 26
Patch Set 3 : Remove Variant typed class and replace with a concrete class #
Total comments: 6
Patch Set 4 : Remove Writing of Property #
Total comments: 10
Patch Set 5 : Fix nits #Patch Set 6 : Fix mysterious build failure #
Messages
Total messages: 28 (9 generated)
dtapuska@chromium.org changed reviewers: + benchan@chromium.org, satorux@chromium.org, wiley@chromium.org
ping.
wtc@chromium.org changed reviewers: + wtc@chromium.org
Review comments on patch set 1: This is an unsolicited drive-by review. Most of my comments are on coding style nits. Also, I didn't read privet_daemon_manager_client.cc, which has the bulk of the code. https://codereview.chromium.org/996013003/diff/1/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/996013003/diff/1/chromeos/chromeos.gyp#newcod... chromeos/chromeos.gyp:182: 'dbus/fake_privet_daemon_manager_client.h', There are two *.gn* files in the chromeos/ directory: chromeos/BUILD.gn chromeos/ime/BUILD.gn Should they be updated? https://codereview.chromium.org/996013003/diff/1/chromeos/chromeos.gyp#newcod... chromeos/chromeos.gyp:234: 'dbus/privet_daemon_manager_client.h', Nit: list "private_*" after "power_*". https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_client_bu... File chromeos/dbus/dbus_client_bundle.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_client_bu... chromeos/dbus/dbus_client_bundle.cc:123: { "privet_daemon", DBusClientBundle::PRIVET_DAEMON }, Nit: list "privet_daemon" after "power_manager". https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... chromeos/dbus/dbus_thread_manager.cc:280: PrivetDaemonManagerClient* DBusThreadManager::GetPrivetDaemonManagerClient() { Nit: list "Private" after "Power". This also applies to the two changes below. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... File chromeos/dbus/dbus_thread_manager.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... chromeos/dbus/dbus_thread_manager.h:60: class PrivetDaemonManagerClient; Nit: list PrivetDaemonManagerClient after PowerManagerClient. This also applies to the two changes below. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... File chromeos/dbus/fake_privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.h:10: #include <vector> Nit: we don't need to include these headers. std::map and std::vector are not used at all. std::string is used, but we can assume it is included by the base class's header because it is only used in an overridden method. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:4: #ifndef CHROMEOS_DBUS_PRIVET_DAEMON_MANAGER_CLIENT_H_ Nit: add a blank line before this line. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:34: std::vector<uint8_t> blob; Nit: this is usually defined as a union in C code. I don't know what is the C++ way, but only one of |string| and |blob| is used, depending on the value of |type|. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:72: const std::string& name() const { return name_.value(); } Just wanted to make sure you considered the issue of whether these strings need to be internationalized. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:97: virtual void ManagerPropertyChanged(const std::string& property_name); Should these methods be declared as abstract (= 0)? https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:107: // events. Check the |object_path| parameter of observer methods to |object_path| is not used in the Observer interface. Did you mean |property_name|?
satorux@chromium.org changed reviewers: + hashimoto@chromium.org - satorux@chromium.org
Took a quick look, but handing over the review for chromeos/dbsu to hashimoto@ who is a better reviewer. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... File chromeos/dbus/fake_privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:11: namespace { nit: add a blank line here https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:14: } a blank line here. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:79: } This and the next function seem to be complex. how about adding unit tests? https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:112: } nit: blank line here. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:34: std::vector<uint8_t> blob; Do we need to have two different types for the storage? I thought std::string and std::vector<uint8_t> were essentially the same thing.
https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... File chromeos/dbus/fake_privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:12: void VoidDBBusMethodCallbackThunk(const VoidDBusMethodCallback& callback) { nit: s/DBBus/DBus/ https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:36: FROM_HERE, base::Bind(&VoidDBBusMethodCallbackThunk, callback)); You don't need VoidDBBusMethodCallbackThunk because you can use base::Bind(callback, DBUS_METHOD_CALL_SUCCESS) instead. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:18: namespace dbus { Generally speaking, you shouldn't add arbitrary stuffs to other namespaces. If you really want to let dbus::PropertySet handle types under namespace chromeos, you should change dbus::PropertySet's design. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:119: const char kPrivetdServiceName[] = "org.chromium.privetd"; It's better to put these constants in the anonymous namespace below as long as they are not used in other files. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:300: PrivetDaemonManagerClient::Observer::~Observer() { If you want to provide empty implementation for interface methods rather than using "=0", you can do it in the header. Please consult existing observer classes as examples. (https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=c...)
https://codereview.chromium.org/996013003/diff/1/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/996013003/diff/1/chromeos/chromeos.gyp#newcod... chromeos/chromeos.gyp:182: 'dbus/fake_privet_daemon_manager_client.h', On 2015/03/17 17:31:05, wtc wrote: > > There are two *.gn* files in the chromeos/ directory: > chromeos/BUILD.gn > chromeos/ime/BUILD.gn > > Should they be updated? No. these BUILD.gn files use a gyp to gn converter. So updating only the gyp file is necessary. https://codereview.chromium.org/996013003/diff/1/chromeos/chromeos.gyp#newcod... chromeos/chromeos.gyp:234: 'dbus/privet_daemon_manager_client.h', On 2015/03/17 17:31:05, wtc wrote: > > Nit: list "private_*" after "power_*". Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_client_bu... File chromeos/dbus/dbus_client_bundle.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_client_bu... chromeos/dbus/dbus_client_bundle.cc:123: { "privet_daemon", DBusClientBundle::PRIVET_DAEMON }, On 2015/03/17 17:31:05, wtc wrote: > > Nit: list "privet_daemon" after "power_manager". Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... File chromeos/dbus/dbus_thread_manager.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... chromeos/dbus/dbus_thread_manager.cc:280: PrivetDaemonManagerClient* DBusThreadManager::GetPrivetDaemonManagerClient() { On 2015/03/17 17:31:05, wtc wrote: > > Nit: list "Private" after "Power". > > This also applies to the two changes below. I believe I was using a different alphabet the day I wrote this; *mud on face* :-( https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... File chromeos/dbus/dbus_thread_manager.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/dbus_thread_ma... chromeos/dbus/dbus_thread_manager.h:60: class PrivetDaemonManagerClient; On 2015/03/17 17:31:05, wtc wrote: > > Nit: list PrivetDaemonManagerClient after PowerManagerClient. > > This also applies to the two changes below. Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... File chromeos/dbus/fake_privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:11: namespace { On 2015/03/17 17:39:10, satorux1 wrote: > nit: add a blank line here Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:12: void VoidDBBusMethodCallbackThunk(const VoidDBusMethodCallback& callback) { On 2015/03/18 05:52:50, hashimoto wrote: > nit: s/DBBus/DBus/ Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:14: } On 2015/03/17 17:39:10, satorux1 wrote: > a blank line here. Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.cc:36: FROM_HERE, base::Bind(&VoidDBBusMethodCallbackThunk, callback)); On 2015/03/18 05:52:50, hashimoto wrote: > You don't need VoidDBBusMethodCallbackThunk because you can use > base::Bind(callback, DBUS_METHOD_CALL_SUCCESS) instead. Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... File chromeos/dbus/fake_privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/fake_privet_da... chromeos/dbus/fake_privet_daemon_manager_client.h:10: #include <vector> On 2015/03/17 17:31:05, wtc wrote: > > Nit: we don't need to include these headers. std::map and std::vector are not > used at all. std::string is used, but we can assume it is included by the base > class's header because it is only used in an overridden method. Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:18: namespace dbus { On 2015/03/18 05:52:50, hashimoto wrote: > Generally speaking, you shouldn't add arbitrary stuffs to other namespaces. > > If you really want to let dbus::PropertySet handle types under namespace > chromeos, you should change dbus::PropertySet's design. I won't use Property template; but use the PropertyBase class; and provide my own implementation of that. It is a cleaner approach. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:79: } On 2015/03/17 17:39:11, satorux1 wrote: > This and the next function seem to be complex. how about adding unit tests? Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:112: } On 2015/03/17 17:39:11, satorux1 wrote: > nit: blank line here. Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:119: const char kPrivetdServiceName[] = "org.chromium.privetd"; On 2015/03/18 05:52:50, hashimoto wrote: > It's better to put these constants in the anonymous namespace below as long as > they are not used in other files. Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.cc:300: PrivetDaemonManagerClient::Observer::~Observer() { On 2015/03/18 05:52:50, hashimoto wrote: > If you want to provide empty implementation for interface methods rather than > using "=0", you can do it in the header. > Please consult existing observer classes as examples. > (https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=c...) Empty implementations should not be done in the header; as per OWNER comments in the following review... https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership... I'll make them pure virtual and put the dtor in the cc file. This should follow all guides/comments. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:4: #ifndef CHROMEOS_DBUS_PRIVET_DAEMON_MANAGER_CLIENT_H_ On 2015/03/17 17:31:05, wtc wrote: > > Nit: add a blank line before this line. Done. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:34: std::vector<uint8_t> blob; On 2015/03/17 17:31:05, wtc wrote: > > Nit: this is usually defined as a union in C code. I don't know what is the C++ > way, but only one of |string| and |blob| is used, depending on the value of > |type|. before C++ 11 you weren't allowed to put non-POD types in a union. It wasn't clear in the style guide if I can use unions of non-POD types; the guide declares if it is not explicitly called out don't use it. This is why I did it this way. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:34: std::vector<uint8_t> blob; On 2015/03/17 17:39:11, satorux1 wrote: > Do we need to have two different types for the storage? I thought std::string > and std::vector<uint8_t> were essentially the same thing. Well yes they are an array of bytes (although one is signed and the other isn't). What I am specifically calling out is that std::string is likely printable; and although I could put the binary code in a string; I felt that then somebody would feel it is printable which it certainly isn't. ie; it isn't ASCII or UTF-8; it is a blob of binary data. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:72: const std::string& name() const { return name_.value(); } On 2015/03/17 17:31:05, wtc wrote: > > Just wanted to make sure you considered the issue of whether these strings need > to be internationalized. These are coming from the dbus API.. I don't have much influence putting an interface on top of it.. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:97: virtual void ManagerPropertyChanged(const std::string& property_name); On 2015/03/17 17:31:05, wtc wrote: > > Should these methods be declared as abstract (= 0)? According to Google C++ style likely. Although the general pattern in the chromeos/dbus classes were not to make them completely abstract. https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:107: // events. Check the |object_path| parameter of observer methods to On 2015/03/17 17:31:05, wtc wrote: > > |object_path| is not used in the Observer interface. Did you mean > |property_name|? Done.
https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:34: std::vector<uint8_t> blob; On 2015/03/18 14:28:54, Dave Tapuska wrote: > On 2015/03/17 17:39:11, satorux1 wrote: > > Do we need to have two different types for the storage? I thought std::string > > and std::vector<uint8_t> were essentially the same thing. > > Well yes they are an array of bytes (although one is signed and the other > isn't). > > What I am specifically calling out is that std::string is likely printable; and > although I could put the binary code in a string; I felt that then somebody > would feel it is printable which it certainly isn't. ie; it isn't ASCII or > UTF-8; it is a blob of binary data. How about replacing this map of variant with a class like this? class ParingInfo { public: std::string sessionId() const; std::string mode() const; std::string code() const; ... }; If you really want to have a variant type in C++, there is already base::Value. If you use base::Value, you can use base::DictionaryValue instead of std::map. You can also use functions declared in dbus/values_util.h to pop/append base::Values to D-Bus messages. https://codereview.chromium.org/996013003/diff/20001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/chromeos.gyp#ne... chromeos/chromeos.gyp:464: 'dbus/privet_daemon_manager_client_unittest.cc', Did you forget to include this file to the patch set? https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/fake_prive... File chromeos/dbus/fake_privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/fake_prive... chromeos/dbus/fake_privet_daemon_manager_client.cc:11: namespace {} // namespace nit: This line is not needed. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:32: const char kNameProperty[] = "Name"; nit: How about sorting these constants (at least within property names) lexicographically? https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:12: #include "base/callback.h" nit: Seems unused. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:14: #include "base/values.h" ditto. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:37: bool operator==( Our style guide disallows operator overloading in general. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Ove... https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:48: chromeos::PrivetDaemonManagerClient::PairingInfoVariant>; nit: "chromeos::PrivetDaemonManagerClient::" is not needed. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:59: PairingInfoVariantMap set_value_; It seems this member is non-empty only in tests. Is this needed? https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:63: class ManagerProperties : public dbus::PropertySet { nit: PrivetDaemonManagerClient::ManagerProperties sounds a bit redundant. How about just calling this class Properties? https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:83: // State of device pairing. The map will contain the following keys. Why don't you just provide getter methods for each property listed below? https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:88: // "mode" value. See design document. > "See design document" Please add a URL to a publicly available document. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:89: const std::map<std::string, PairingInfoVariant>& pairing_info() const { nit: Please use the typedef above.
https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/1/chromeos/dbus/privet_daemon_... chromeos/dbus/privet_daemon_manager_client.h:34: std::vector<uint8_t> blob; On 2015/03/19 08:00:22, hashimoto wrote: > On 2015/03/18 14:28:54, Dave Tapuska wrote: > > On 2015/03/17 17:39:11, satorux1 wrote: > > > Do we need to have two different types for the storage? I thought > std::string > > > and std::vector<uint8_t> were essentially the same thing. > > > > Well yes they are an array of bytes (although one is signed and the other > > isn't). > > > > What I am specifically calling out is that std::string is likely printable; > and > > although I could put the binary code in a string; I felt that then somebody > > would feel it is printable which it certainly isn't. ie; it isn't ASCII or > > UTF-8; it is a blob of binary data. > > How about replacing this map of variant with a class like this? > class ParingInfo { > public: > std::string sessionId() const; > std::string mode() const; > std::string code() const; > ... > }; > > If you really want to have a variant type in C++, there is already base::Value. > If you use base::Value, you can use base::DictionaryValue instead of std::map. > You can also use functions declared in dbus/values_util.h to pop/append > base::Values to D-Bus messages. I wasn't sure what would be the desired approach here. Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/chromeos.gyp#ne... chromeos/chromeos.gyp:464: 'dbus/privet_daemon_manager_client_unittest.cc', On 2015/03/19 08:00:22, hashimoto wrote: > Did you forget to include this file to the patch set? Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/fake_prive... File chromeos/dbus/fake_privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/fake_prive... chromeos/dbus/fake_privet_daemon_manager_client.cc:11: namespace {} // namespace On 2015/03/19 08:00:22, hashimoto wrote: > nit: This line is not needed. Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:32: const char kNameProperty[] = "Name"; On 2015/03/19 08:00:22, hashimoto wrote: > nit: How about sorting these constants (at least within property names) > lexicographically? Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:12: #include "base/callback.h" On 2015/03/19 08:00:23, hashimoto wrote: > nit: Seems unused. Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:14: #include "base/values.h" On 2015/03/19 08:00:23, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:37: bool operator==( On 2015/03/19 08:00:23, hashimoto wrote: > Our style guide disallows operator overloading in general. > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Ove... It was used in the test case; which is permitted because of the STL == operator used on the map. But I've since change this not to use a map per your suggestion. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:48: chromeos::PrivetDaemonManagerClient::PairingInfoVariant>; On 2015/03/19 08:00:23, hashimoto wrote: > nit: "chromeos::PrivetDaemonManagerClient::" is not needed. Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:59: PairingInfoVariantMap set_value_; On 2015/03/19 08:00:23, hashimoto wrote: > It seems this member is non-empty only in tests. > Is this needed? The PropertyBase makes special use of set_value vs value.. specifically there are overrides. I don't want to deviate too far from the desired implementation that PropertyBase teaches. It is mainly here for writing... https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:63: class ManagerProperties : public dbus::PropertySet { On 2015/03/19 08:00:23, hashimoto wrote: > nit: PrivetDaemonManagerClient::ManagerProperties sounds a bit redundant. > How about just calling this class Properties? Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:83: // State of device pairing. The map will contain the following keys. On 2015/03/19 08:00:23, hashimoto wrote: > Why don't you just provide getter methods for each property listed below? Done. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:88: // "mode" value. See design document. On 2015/03/19 08:00:23, hashimoto wrote: > > "See design document" > Please add a URL to a publicly available document. Adjusted comments per class rewrite. https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:89: const std::map<std::string, PairingInfoVariant>& pairing_info() const { On 2015/03/19 08:00:23, hashimoto wrote: > nit: Please use the typedef above. Done.
https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:59: PairingInfoVariantMap set_value_; On 2015/03/19 18:04:24, Dave Tapuska wrote: > On 2015/03/19 08:00:23, hashimoto wrote: > > It seems this member is non-empty only in tests. > > Is this needed? > > The PropertyBase makes special use of set_value vs value.. specifically there > are overrides. I don't want to deviate too far from the desired implementation > that PropertyBase teaches. It is mainly here for writing... What I'm curious about is if there is any chance AppendSetValueToWriter(), ReplaceValueWithSetValue() and |set_value_| get used. IIUC ParingInfoProperty is used in read-only fashion because Properties doesn't provide non-const access to |paring_info_|. If we don't need write-access of this property for foreseeable future, there is no reason to keep unused code being maintained. You should remove |set_value_|, and the related methods should be just marked as NOTIMPLEMENTED() if there is no possibility they get actually used. (It's unfortunate dbus::PropertySet doesn't provide a way to implement read-only property cleanly, but that's another topic.) https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:16: #include "dbus/values_util.h" nit: Looks unused. https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client_unittest.cc (right): https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2015. https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:11: #include "third_party/cros_system_api/dbus/service_constants.h" nit: Looks unused.
https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/20001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:59: PairingInfoVariantMap set_value_; On 2015/03/20 06:41:48, hashimoto wrote: > On 2015/03/19 18:04:24, Dave Tapuska wrote: > > On 2015/03/19 08:00:23, hashimoto wrote: > > > It seems this member is non-empty only in tests. > > > Is this needed? > > > > The PropertyBase makes special use of set_value vs value.. specifically there > > are overrides. I don't want to deviate too far from the desired implementation > > that PropertyBase teaches. It is mainly here for writing... > > What I'm curious about is if there is any chance AppendSetValueToWriter(), > ReplaceValueWithSetValue() and |set_value_| get used. > IIUC ParingInfoProperty is used in read-only fashion because Properties doesn't > provide non-const access to |paring_info_|. > > If we don't need write-access of this property for foreseeable future, there is > no reason to keep unused code being maintained. > You should remove |set_value_|, and the related methods should be just marked as > NOTIMPLEMENTED() if there is no possibility they get actually used. > (It's unfortunate dbus::PropertySet doesn't provide a way to implement read-only > property cleanly, but that's another topic.) Done. https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:16: #include "dbus/values_util.h" On 2015/03/20 06:41:48, hashimoto wrote: > nit: Looks unused. Done. https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client_unittest.cc (right): https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/03/20 06:41:48, hashimoto wrote: > nit: 2015. Done. https://codereview.chromium.org/996013003/diff/40001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:11: #include "third_party/cros_system_api/dbus/service_constants.h" On 2015/03/20 06:41:48, hashimoto wrote: > nit: Looks unused. Done.
lgtm with nits. https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:192: dbus::MessageReader variant_reader(NULL); nit: Please prefer nullptr to NULL. The same goes for the unit test. https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:198: value_.Clear(); suggestion: How about clearing |value_| at the beginning of this function so that |value_| is always empty when this function returns false? https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:103: // Interface for observing changes from a apmanager daemon. nit: Why is "apmanager daemon" mentioned here and below? Please add a brief description about it. https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client_unittest.cc (right): https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:14: TEST(PrivetDaemonManagerClientTest, ReadWritePairingInfo) { nit: s/ReadWrite/Read/ ? https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:49: arraysize(code_value))); suggestion: Maybe EXPECT_EQ(std::vector<uint8_t>(code_value, code_value + arraysize(code_value)), parint_info.value().code()); is simpler?
https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.cc (right): https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:192: dbus::MessageReader variant_reader(NULL); On 2015/03/20 15:22:57, hashimoto wrote: > nit: Please prefer nullptr to NULL. > The same goes for the unit test. Done. https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.cc:198: value_.Clear(); On 2015/03/20 15:22:57, hashimoto wrote: > suggestion: How about clearing |value_| at the beginning of this function so > that |value_| is always empty when this function returns false? Done. https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client.h (right): https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client.h:103: // Interface for observing changes from a apmanager daemon. On 2015/03/20 15:22:57, hashimoto wrote: > nit: Why is "apmanager daemon" mentioned here and below? Please add a brief > description about it. Done. https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... File chromeos/dbus/privet_daemon_manager_client_unittest.cc (right): https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:14: TEST(PrivetDaemonManagerClientTest, ReadWritePairingInfo) { On 2015/03/20 15:22:57, hashimoto wrote: > nit: s/ReadWrite/Read/ ? Done. https://codereview.chromium.org/996013003/diff/60001/chromeos/dbus/privet_dae... chromeos/dbus/privet_daemon_manager_client_unittest.cc:49: arraysize(code_value))); On 2015/03/20 15:22:57, hashimoto wrote: > suggestion: Maybe EXPECT_EQ(std::vector<uint8_t>(code_value, code_value + > arraysize(code_value)), parint_info.value().code()); is simpler? Done.
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/996013003/#ps80001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996013003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/03/20 17:07:00, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) although I agree with the build failure; I'm not sure why it isn't generated in my env. I have build chromeos release & debug with gn and gyp and don't see it.
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/996013003/#ps100001 (title: "Fix mysterious build failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996013003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ab7987e2b286aead19236e89718f8cd97b242ec1 Cr-Commit-Position: refs/heads/master@{#321621}
Message was sent while issue was closed.
On 2015/03/20 19:17:45, Dave Tapuska wrote: > On 2015/03/20 17:07:00, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > although I agree with the build failure; I'm not sure why it isn't generated in > my env. I have build chromeos release & debug with gn and gyp and don't see it. We have a special style checker plugin for clang. http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
