Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
893663002:
Enhance the DBus interface for peerd (Closed)
|
Created:
5 years, 10 months ago by dtapuska Modified:
5 years, 10 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. |
DescriptionEnhance the DBus interface for peerd
Add ability to query properties of
- Manager
- Peer
- Service
Add Observer to peerd so we are told when things change.
BUG=453873
Committed: https://crrev.com/32d25453c4d04474d75a6db01cd1e006537bd3bf
Cr-Commit-Position: refs/heads/master@{#315311}
Patch Set 1 #
Total comments: 45
Patch Set 2 : Address comments from patch 1 #
Total comments: 14
Patch Set 3 : Fix comments from patch 2 #Patch Set 4 : Minor comment update #Patch Set 5 : Fix issues with last patch #
Total comments: 3
Patch Set 6 : Fix minor nits #
Messages
Total messages: 17 (5 generated)
dtapuska@chromium.org changed reviewers: + benchan@chromium.org, pwestin@chromium.org, stevenjb@chromium.org, wiley@chromium.org
On 2015/01/30 19:09:00, Dave Tapuska wrote: > mailto:dtapuska@chromium.org changed reviewers: > + mailto:benchan@chromium.org, mailto:pwestin@chromium.org, mailto:stevenjb@chromium.org, > mailto:wiley@chromium.org Ping
stevenjb@chromium.org changed reviewers: + satorux@chromium.org
-> satorux@ for src/dbus changes
satorux@chromium.org changed reviewers: + hashimoto@chromium.org
hashimoto@ could you also take a look? https://codereview.chromium.org/893663002/diff/1/dbus/message.cc File dbus/message.cc (right): https://codereview.chromium.org/893663002/diff/1/dbus/message.cc#newcode840 dbus/message.cc:840: strings->push_back(std::move(string)); This change seems to be unrelated to the purpose of the patch. If needed, maybe do this in a separate patch? https://codereview.chromium.org/893663002/diff/1/dbus/property.cc File dbus/property.cc (right): https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode578 dbus/property.cc:578: } The new functions have lots of logic. Could you add unit tests for these?
https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/dbus_client_bu... File chromeos/dbus/dbus_client_bundle.h (right): https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/dbus_client_bu... chromeos/dbus/dbus_client_bundle.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Nice cleanup, but I think this part is out of this CL's scope. Could you split cleanup part to a different CL? (the same goes for dbus_client_bundle.cc) https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... File chromeos/dbus/peer_daemon_manager_client.cc (right): https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:27: const char kServiceInterface[] = "org.chromium.peerd.Service"; nit: As noted in the header, please be consistent whether things are ordered like Manager->Peer->Service or Manager->Service->Peer. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:35: namespace { unnamed namespace should start from before string constants above. No need to give them external linkage. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:71: // dbus::ObjectManager::Interface override. nit: "overrides:" instead of "override." to be consistent with other parts. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:107: object_manager_->UnregisterInterface("org.chromium.leaderd.Manager"); This code doesn't match with RegisterInterface() calls below. Please use constants. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:151: callback.Run(DBUS_METHOD_CALL_FAILURE, ""); nit: Please use std::string() instead of "" (cf. https://codereview.chromium.org/13145003). Please PostTask() instead of running the callback directly. The same goes for other parts. In the example below, the caller expects B() to run after A() (and B() runs after A() in usual cases), but by directly running the callback here, it can be A() after B() when something goes wrong. This may result in a bug difficult to find which only happens in extremely rare use cases. void f(PeerDaemonManagerClientImpl* p) { p->StartMonitoring(base::Bind(&B)); A(); } https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:277: } How about leaving a log when interface_name is an unexpected value? The same goes for ObjectAdded() and ObjectRemoved(). https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... File chromeos/dbus/peer_daemon_manager_client.h (right): https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:10: #include <tuple> <tuple> is not allowed yet. http://chromium-cpp.appspot.com/ https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:33: dbus::Property<std::vector<std::string>> monitored_technologies; Could you make this and other properties private? https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:60: dbus::Property<std::string> u_u_i_d; nit: Seems |uuid| is more common. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:119: // Retries a list of all the peers. nit: Retrieves? https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:122: // Retries a list of all the services. ditto. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:125: // Obtain the properties for the peer with object path |object_path|, nit: Obtains https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:130: // Obtain the properties for the service with object path |object_path|, ditto. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:132: virtual ServiceProperties* GetServiceProperties( nit: Properties and Observer methods are ordered like Manager->Service->Peer, but in this part it's Peer->Service. It should be better to be consistent everywhere. https://codereview.chromium.org/893663002/diff/1/dbus/message.cc File dbus/message.cc (right): https://codereview.chromium.org/893663002/diff/1/dbus/message.cc#newcode840 dbus/message.cc:840: strings->push_back(std::move(string)); On 2015/02/05 08:20:15, satorux1 wrote: > This change seems to be unrelated to the purpose of the patch. If needed, maybe > do this in a separate patch? move semantics is not allowed yet. http://chromium-cpp.appspot.com/ https://codereview.chromium.org/893663002/diff/1/dbus/property.cc File dbus/property.cc (right): https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode482 dbus/property.cc:482: // Property<std::map<std::string, std::string> > specialization. nit: No need for space in "> >". The same goes for other places. https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode488 dbus/property.cc:488: MessageReader array_reader(NULL); nit: Please move this line to the line before PopArray() to avoid unnecessary initialization on error cases. (or if you prefer less lines of code, you can keep this line here while merging two "if"s below by ||.) The same goes for other parts. https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode504 dbus/property.cc:504: value_.emplace(std::move(key), std::move(value)); move is not allowed yet. http://chromium-cpp.appspot.com/ https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode528 dbus/property.cc:528: // Propertystd::vector<std::tuple<std::vector<uint8_t>, uint16_t> > > nit: Property<std::vector<std::tuple<std::vector<uint8_t>, uint16_t>>> https://codereview.chromium.org/893663002/diff/1/dbus/property.h File dbus/property.h (right): https://codereview.chromium.org/893663002/diff/1/dbus/property.h#newcode10 dbus/property.h:10: #include <tuple> <tuple> is a C++11 library feature which is not still allowed.
https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... File chromeos/dbus/peer_daemon_manager_client.cc (right): https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:27: const char kServiceInterface[] = "org.chromium.peerd.Service"; On 2015/02/05 09:51:11, hashimoto wrote: > nit: As noted in the header, please be consistent whether things are ordered > like Manager->Peer->Service or Manager->Service->Peer. Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:35: namespace { On 2015/02/05 09:51:11, hashimoto wrote: > unnamed namespace should start from before string constants above. > No need to give them external linkage. Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:71: // dbus::ObjectManager::Interface override. On 2015/02/05 09:51:11, hashimoto wrote: > nit: "overrides:" instead of "override." to be consistent with other parts. Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:107: object_manager_->UnregisterInterface("org.chromium.leaderd.Manager"); On 2015/02/05 09:51:11, hashimoto wrote: > This code doesn't match with RegisterInterface() calls below. > > Please use constants. Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:151: callback.Run(DBUS_METHOD_CALL_FAILURE, ""); On 2015/02/05 09:51:11, hashimoto wrote: > nit: Please use std::string() instead of "" (cf. > https://codereview.chromium.org/13145003). > > > Please PostTask() instead of running the callback directly. > The same goes for other parts. > > In the example below, the caller expects B() to run after A() (and B() runs > after A() in usual cases), but by directly running the callback here, it can be > A() after B() when something goes wrong. > This may result in a bug difficult to find which only happens in extremely rare > use cases. > > void f(PeerDaemonManagerClientImpl* p) { > p->StartMonitoring(base::Bind(&B)); > A(); > } Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.cc:277: } On 2015/02/05 09:51:11, hashimoto wrote: > How about leaving a log when interface_name is an unexpected value? > The same goes for ObjectAdded() and ObjectRemoved(). Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... File chromeos/dbus/peer_daemon_manager_client.h (right): https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:10: #include <tuple> On 2015/02/05 09:51:12, hashimoto wrote: > <tuple> is not allowed yet. http://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:33: dbus::Property<std::vector<std::string>> monitored_technologies; On 2015/02/05 09:51:12, hashimoto wrote: > Could you make this and other properties private? I'd need to add access methods for these properties. I followed the bluetooth_device_client.h Would you prefer I converted this into a struct to exactly follow that code? https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:60: dbus::Property<std::string> u_u_i_d; On 2015/02/05 09:51:12, hashimoto wrote: > nit: Seems |uuid| is more common. Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:119: // Retries a list of all the peers. On 2015/02/05 09:51:12, hashimoto wrote: > nit: Retrieves? Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:122: // Retries a list of all the services. On 2015/02/05 09:51:12, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:125: // Obtain the properties for the peer with object path |object_path|, On 2015/02/05 09:51:12, hashimoto wrote: > nit: Obtains Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:130: // Obtain the properties for the service with object path |object_path|, On 2015/02/05 09:51:12, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:132: virtual ServiceProperties* GetServiceProperties( On 2015/02/05 09:51:12, hashimoto wrote: > nit: Properties and Observer methods are ordered like Manager->Service->Peer, > but in this part it's Peer->Service. > It should be better to be consistent everywhere. Done. https://codereview.chromium.org/893663002/diff/1/dbus/message.cc File dbus/message.cc (right): https://codereview.chromium.org/893663002/diff/1/dbus/message.cc#newcode840 dbus/message.cc:840: strings->push_back(std::move(string)); On 2015/02/05 09:51:12, hashimoto wrote: > On 2015/02/05 08:20:15, satorux1 wrote: > > This change seems to be unrelated to the purpose of the patch. If needed, > maybe > > do this in a separate patch? > > move semantics is not allowed yet. > http://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/893663002/diff/1/dbus/property.cc File dbus/property.cc (right): https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode482 dbus/property.cc:482: // Property<std::map<std::string, std::string> > specialization. On 2015/02/05 09:51:12, hashimoto wrote: > nit: No need for space in "> >". > The same goes for other places. Done. https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode488 dbus/property.cc:488: MessageReader array_reader(NULL); On 2015/02/05 09:51:12, hashimoto wrote: > nit: Please move this line to the line before PopArray() to avoid unnecessary > initialization on error cases. > (or if you prefer less lines of code, you can keep this line here while merging > two "if"s below by ||.) > The same goes for other parts. Done. https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode504 dbus/property.cc:504: value_.emplace(std::move(key), std::move(value)); On 2015/02/05 09:51:12, hashimoto wrote: > move is not allowed yet. http://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/893663002/diff/1/dbus/property.cc#newcode528 dbus/property.cc:528: // Propertystd::vector<std::tuple<std::vector<uint8_t>, uint16_t> > > On 2015/02/05 09:51:12, hashimoto wrote: > nit: Property<std::vector<std::tuple<std::vector<uint8_t>, uint16_t>>> Done. https://codereview.chromium.org/893663002/diff/1/dbus/property.h File dbus/property.h (right): https://codereview.chromium.org/893663002/diff/1/dbus/property.h#newcode10 dbus/property.h:10: #include <tuple> On 2015/02/05 09:51:12, hashimoto wrote: > <tuple> is a C++11 library feature which is not still allowed. Done.
Thanks, looking good. https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... File chromeos/dbus/peer_daemon_manager_client.h (right): https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:33: dbus::Property<std::vector<std::string>> monitored_technologies; On 2015/02/05 19:51:17, Dave Tapuska wrote: > On 2015/02/05 09:51:12, hashimoto wrote: > > Could you make this and other properties private? > > I'd need to add access methods for these properties. I followed the > bluetooth_device_client.h > > Would you prefer I converted this into a struct to exactly follow that code? Generally speaking, we shouldn't expose data members public. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control) It's always better to explicitly provide setter methods when write-access is allowed. If you are sure you want to allow write-access of all properties for random user code, making this a struct to be consistent with the existing Bluetooth example might be OK. (Though, struct consisting of non-POD members look kind of unnatural IMHO.) https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... File chromeos/dbus/peer_daemon_manager_client.cc (right): https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:55: const dbus::ObjectPath& object_path) override; nit: Please put these overrides in the same order as declared in the header. https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:87: void OnPeerPropertyChanged(const dbus::ObjectPath& object_path, nit: Please stick exclusively to one ordering, Manager->Peer->Service, or Manager->Service->Peer to make the code intuitively understandable. The same goes for everywhere. https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:201: callback.Run(DBUS_METHOD_CALL_FAILURE, ""); Please PostTask() here and use std::string(). The same goes for other methods. https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:383: PeerDaemonManagerClient::PeerProperties::PeerProperties( Please put methods in the same order as declared in the header. https://codereview.chromium.org/893663002/diff/20001/dbus/property.cc File dbus/property.cc (right): https://codereview.chromium.org/893663002/diff/20001/dbus/property.cc#newcode527 dbus/property.cc:527: // Propertystd::vector<std::pair<std::vector<uint8_t>, uint16_t>>> nit: "<" after Property is missing . https://codereview.chromium.org/893663002/diff/20001/dbus/property.h File dbus/property.h (right): https://codereview.chromium.org/893663002/diff/20001/dbus/property.h#newcode505 dbus/property.h:505: std::vector<std::pair<std::vector<uint8_t>, uint16_t>>>; It's unfortunate property.h should be changed to support this type (and any tricky template types) :( Anyways, it's OK as it seems this is the only available option under the current design. https://codereview.chromium.org/893663002/diff/20001/dbus/property_unittest.cc File dbus/property_unittest.cc (right): https://codereview.chromium.org/893663002/diff/20001/dbus/property_unittest.c... dbus/property_unittest.cc:306: } Instead of changing test_service.cc, how about adding simpler test cases like: // Construct message. dbus::Message message; MessageWriter write(&message)); ... MessageReader reader(&message); Property<std::map<std::string, std::string>> property; EXPECT_TRUE(property.PopValueFromReader(&reader)); // Verify the result. ... You can add a setter for set_value to test AppendSetValueToWriter(). FYI: By including strings like "ForTesting" in the setter function name, you can prevent improper usage in code other than tests. https://cs.chromium.org/file:src/PRESUBMIT.py%20ForTest
https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... File chromeos/dbus/peer_daemon_manager_client.h (right): https://codereview.chromium.org/893663002/diff/1/chromeos/dbus/peer_daemon_ma... chromeos/dbus/peer_daemon_manager_client.h:33: dbus::Property<std::vector<std::string>> monitored_technologies; On 2015/02/06 07:45:11, hashimoto wrote: > On 2015/02/05 19:51:17, Dave Tapuska wrote: > > On 2015/02/05 09:51:12, hashimoto wrote: > > > Could you make this and other properties private? > > > > I'd need to add access methods for these properties. I followed the > > bluetooth_device_client.h > > > > Would you prefer I converted this into a struct to exactly follow that code? > > Generally speaking, we shouldn't expose data members public. > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control) > It's always better to explicitly provide setter methods when write-access is > allowed. > > If you are sure you want to allow write-access of all properties for random user > code, making this a struct to be consistent with the existing Bluetooth example > might be OK. > (Though, struct consisting of non-POD members look kind of unnatural IMHO.) Done. https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... File chromeos/dbus/peer_daemon_manager_client.cc (right): https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:55: const dbus::ObjectPath& object_path) override; On 2015/02/06 07:45:11, hashimoto wrote: > nit: Please put these overrides in the same order as declared in the header. Done... Manager, Service, Peer should be the order everywhere now. https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:87: void OnPeerPropertyChanged(const dbus::ObjectPath& object_path, On 2015/02/06 07:45:11, hashimoto wrote: > nit: Please stick exclusively to one ordering, Manager->Peer->Service, or > Manager->Service->Peer to make the code intuitively understandable. > The same goes for everywhere. Done. https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:201: callback.Run(DBUS_METHOD_CALL_FAILURE, ""); On 2015/02/06 07:45:12, hashimoto wrote: > Please PostTask() here and use std::string(). > The same goes for other methods. Done. https://codereview.chromium.org/893663002/diff/20001/chromeos/dbus/peer_daemo... chromeos/dbus/peer_daemon_manager_client.cc:383: PeerDaemonManagerClient::PeerProperties::PeerProperties( On 2015/02/06 07:45:11, hashimoto wrote: > Please put methods in the same order as declared in the header. Done. https://codereview.chromium.org/893663002/diff/20001/dbus/property.cc File dbus/property.cc (right): https://codereview.chromium.org/893663002/diff/20001/dbus/property.cc#newcode527 dbus/property.cc:527: // Propertystd::vector<std::pair<std::vector<uint8_t>, uint16_t>>> On 2015/02/06 07:45:12, hashimoto wrote: > nit: "<" after Property is missing . Done. https://codereview.chromium.org/893663002/diff/20001/dbus/property.h File dbus/property.h (right): https://codereview.chromium.org/893663002/diff/20001/dbus/property.h#newcode505 dbus/property.h:505: std::vector<std::pair<std::vector<uint8_t>, uint16_t>>>; On 2015/02/06 07:45:12, hashimoto wrote: > It's unfortunate property.h should be changed to support this type (and any > tricky template types) :( > Anyways, it's OK as it seems this is the only available option under the current > design. Acknowledged. https://codereview.chromium.org/893663002/diff/20001/dbus/property_unittest.cc File dbus/property_unittest.cc (right): https://codereview.chromium.org/893663002/diff/20001/dbus/property_unittest.c... dbus/property_unittest.cc:306: } On 2015/02/06 07:45:12, hashimoto wrote: > Instead of changing test_service.cc, how about adding simpler test cases like: > > // Construct message. > dbus::Message message; > MessageWriter write(&message)); > ... > MessageReader reader(&message); > > Property<std::map<std::string, std::string>> property; > EXPECT_TRUE(property.PopValueFromReader(&reader)); > > // Verify the result. > ... > > You can add a setter for set_value to test AppendSetValueToWriter(). > FYI: By including strings like "ForTesting" in the setter function name, you can > prevent improper usage in code other than tests. > https://cs.chromium.org/file:src/PRESUBMIT.py%20ForTest Done.
lgtm. satorux@, could you review this change as an owner of src/dbus? https://codereview.chromium.org/893663002/diff/80001/dbus/property_unittest.cc File dbus/property_unittest.cc (right): https://codereview.chromium.org/893663002/diff/80001/dbus/property_unittest.c... dbus/property_unittest.cc:355: EXPECT_EQ(5U, item.first.size()); nit: This should be ASSERT, otherwise the test may crash when item.first.size() < 5. https://codereview.chromium.org/893663002/diff/80001/dbus/property_unittest.c... dbus/property_unittest.cc:356: EXPECT_EQ('T', item.first[0]); nit: Why don't you compare the value with |ip_bytes|?
dbus/ LGTM with hashimoto's comments addressed. https://codereview.chromium.org/893663002/diff/80001/dbus/property_unittest.cc File dbus/property_unittest.cc (right): https://codereview.chromium.org/893663002/diff/80001/dbus/property_unittest.c... dbus/property_unittest.cc:386: } Thank you for adding these tests!
New patchsets have been uploaded after l-g-t-m from hashimoto@chromium.org,satorux@chromium.org
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/893663002/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/32d25453c4d04474d75a6db01cd1e006537bd3bf Cr-Commit-Position: refs/heads/master@{#315311} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
