Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
681723003:
Add new shill client for VPN (Closed)
|
Created:
6 years, 1 month ago by kaliamoorthi Modified:
6 years, 1 month ago CC:
chromium-reviews, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org, Andrew T Wilson (Slow), bartfab (slow), Paul Stewart Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd new shill client for VPN
This CL adds new shill client for third party vpn dbus interface.
BUG=407541
Committed: https://crrev.com/4c839047c821a5109fd8b5b828f1ca2082cca07a
Cr-Commit-Position: refs/heads/master@{#304054}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Extended to include related changes in dbus and adds a fake client #
Total comments: 14
Patch Set 3 : Rebased and fixed previous reveiew comments. #Patch Set 4 : Added unit tests #
Total comments: 14
Patch Set 5 : Fixes review comments from Steven #
Total comments: 8
Patch Set 6 : Rebased and fixed nits #Patch Set 7 : Updated the interface to better suit the upper layer #
Total comments: 78
Patch Set 8 : Fixes comments from Philipp #Patch Set 9 : Removes dependence on basic_types.h and fixes a build error #
Total comments: 16
Patch Set 10 : Fixes comments from Philipp and Steven #
Total comments: 2
Patch Set 11 : Fixes nit from Bartosz #
Total comments: 6
Patch Set 12 : Fixes comments from Philipp #
Total comments: 4
Patch Set 13 : Fixes the issue in unit tests #
Total comments: 2
Patch Set 14 : Fixes nit from Steven #Messages
Total messages: 45 (11 generated)
kaliamoorthi@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven, Following our initial discussion on VPN, I am prototyping a solution for it. I have made changes to shill to support a new DBUS interface for this (https://chromium-review.googlesource.com/#/c/223690/). I am working on client side code and this CL has an implementation of it. I want to get your initial response. It is untested. I'll refine this CL and add unit tests. PTAL.
'ThirdPartyVpn' is kind of cumbersome, maybe brainstorm other names, maybe 'CustomVpn'? Overall this looks fine. I mostly made style comments, I'll look more closely at the dbus calls next round (but you might want to add someone else familiar with the interface also). https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:29: "dns_servers"}; Can we make these part of the ONC spec? Regardless, these keys should be defined individually in a separate header so that other code can use the consts. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:41: virtual ~ShillThirdPartyVpnDriverClientImpl(); Use the new override style: ~ShillThirdPartyVpnDriverClientImpl() override; + no 'virtual' in the overrides below. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:72: ShillClientHelper* get_helper() { return &helper; } Don't add this, just use &(info->helper), and see other comments. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:88: return it->second->get_helper(); I would have this return a HelperInfo*, otherwise tracking ownership/lifetime of the result of this becomes difficult (i.e. we need to ensure that the result can't be used after the HelperInfo object is destroyed, which is easier to do if we just pass a pointer to the HelperInfo object itself). https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:100: std::set<std::string> valid_keys; valid_keys_ https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:107: : helper(object_proxy), observer(NULL) { One line per member initializer (google_clang_format is your friend) https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:137: if (it != helpers_.end()) { Either {D}CHECK or add a test, not both. (This if would never fail). Since the CHECK could potentially introduce edge cases that result in a crash, I would suggest using LOG(ERROR) and an early return if it == helpers_.end(). https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:165: if (it != helpers_.end()) { Same here. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:176: if (it != helpers_.end()) { Early exit. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:236: if (helper_info->observer) { Early exit. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:249: if (helper_info->observer) { Early exit. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.h:31: }; This should be in a separate file (both for readability and so that observer implementors do not need to include all of this file in their headers). Also needs a protected distructor (can be inlined since this is a pure virtual intervace, i.e. protected: virtual ~ShillThirdPartyVpnObserver() {}.
Patchset #2 (id:20001) has been deleted
kaliamoorthi@chromium.org changed reviewers: + pstew@chromium.org
CustomVpn is short, but all the shill code refers to it as third party vpn. This CL addresses previous comments and adds new changes. Adding pstew@ for reviewing the client side interfaces. PTAL. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:29: "dns_servers"}; On 2014/10/29 18:14:56, stevenjb wrote: > Can we make these part of the ONC spec? Regardless, these keys should be defined > individually in a separate header so that other code can use the consts. We discussed ONC aspect, these values would be set by an VPN extension and the values may not be known upfront so ONC would be an optional addition rather than the only option. In normal operation mode, we would expect these values to be set by the extension possibly controlled by policy for extensions. I added a TODO to move it to service_constants.h. Initially it would be here but once all the constants are finalized I'll move it to service_constants.h https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:41: virtual ~ShillThirdPartyVpnDriverClientImpl(); On 2014/10/29 18:14:56, stevenjb wrote: > Use the new override style: > ~ShillThirdPartyVpnDriverClientImpl() override; > + no 'virtual' in the overrides below. Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:72: ShillClientHelper* get_helper() { return &helper; } On 2014/10/29 18:14:56, stevenjb wrote: > Don't add this, just use &(info->helper), and see other comments. Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:88: return it->second->get_helper(); On 2014/10/29 18:14:56, stevenjb wrote: > I would have this return a HelperInfo*, otherwise tracking ownership/lifetime of > the result of this becomes difficult (i.e. we need to ensure that the result > can't be used after the HelperInfo object is destroyed, which is easier to do if > we just pass a pointer to the HelperInfo object itself). Maybe I don't understand this comment, HelperIno and helper get destroyed together. The only inconsistent state is if HelperInfo is destroyed and there is still a pointer to it in helpers_. Is this the condition you are talking about? This should not happen since when helper_info is deleted, it is also removed from helpers_. We'll discuss this offline. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:100: std::set<std::string> valid_keys; On 2014/10/29 18:14:56, stevenjb wrote: > valid_keys_ Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:107: : helper(object_proxy), observer(NULL) { On 2014/10/29 18:14:56, stevenjb wrote: > One line per member initializer (google_clang_format is your friend) I use clang format already and it formatted it this way. I verified the coding guideline as well, it says if the entire list can fit in a line, it is okay. Otherwise put it in seperate line. If you still insist, I can make this change in the end. As I am modifying the code, I would rely on clang format and it would put it in a single line again, making me repeat the work. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:137: if (it != helpers_.end()) { On 2014/10/29 18:14:56, stevenjb wrote: > Either {D}CHECK or add a test, not both. (This if would never fail). > Since the CHECK could potentially introduce edge cases that result in a crash, I > would suggest using LOG(ERROR) and an early return if it == helpers_.end(). Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:165: if (it != helpers_.end()) { On 2014/10/29 18:14:56, stevenjb wrote: > Same here. Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:176: if (it != helpers_.end()) { On 2014/10/29 18:14:56, stevenjb wrote: > Early exit. Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:236: if (helper_info->observer) { On 2014/10/29 18:14:56, stevenjb wrote: > Early exit. Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:249: if (helper_info->observer) { On 2014/10/29 18:14:56, stevenjb wrote: > Early exit. Done. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.h:31: }; On 2014/10/29 18:14:56, stevenjb wrote: > This should be in a separate file (both for readability and so that observer > implementors do not need to include all of this file in their headers). Also > needs a protected distructor (can be inlined since this is a pure virtual > intervace, i.e. protected: virtual ~ShillThirdPartyVpnObserver() {}. > Done.
https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:29: "dns_servers"}; On 2014/10/30 13:09:05, kaliamoorthi wrote: > On 2014/10/29 18:14:56, stevenjb wrote: > > Can we make these part of the ONC spec? Regardless, these keys should be > defined > > individually in a separate header so that other code can use the consts. > > We discussed ONC aspect, these values would be set by an VPN extension and the > values may not be known upfront so ONC would be an optional addition rather than > the only option. In normal operation mode, we would expect these values to be > set by the extension possibly controlled by policy for extensions. > > I added a TODO to move it to service_constants.h. Initially it would be here but > once all the constants are finalized I'll move it to service_constants.h It just occurred to me that this is lower level than what any extension API will be using, so for now ONC or not doesn't really mater. If Shill needs to parse these properties, then they definitely need to be part of service_constants. We really should define all of these there first, even if they might change later, but for expediency I am OK with defining them here first, but they really need to be defined as individual constants, not just as an array. (And the array itself should be defined in the anonymous namespace). https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:88: return it->second->get_helper(); On 2014/10/30 13:09:04, kaliamoorthi wrote: > On 2014/10/29 18:14:56, stevenjb wrote: > > I would have this return a HelperInfo*, otherwise tracking ownership/lifetime > of > > the result of this becomes difficult (i.e. we need to ensure that the result > > can't be used after the HelperInfo object is destroyed, which is easier to do > if > > we just pass a pointer to the HelperInfo object itself). > > Maybe I don't understand this comment, HelperIno and helper get destroyed > together. The only inconsistent state is if HelperInfo is destroyed and there is > still a pointer to it in helpers_. Is this the condition you are talking about? > This should not happen since when helper_info is deleted, it is also removed > from helpers_. > > We'll discuss this offline. Yes, I understand that ShillClientHelper is owned by HelperInfo and will only be deleted with it, but at first glance this is confusing. Using GetHelperInfo()->helper would make it clear that this function is managing HelperInfo instances, and 'helper' is a member of that. Alternatively you could split this into GetHelperInfo and have GetHelper() return &GetHelperInfo()->helper, but the current code confused me initially. https://codereview.chromium.org/681723003/diff/1/chromeos/dbus/shill_third_pa... chromeos/dbus/shill_third_party_vpn_driver_client.cc:107: : helper(object_proxy), observer(NULL) { On 2014/10/30 13:09:04, kaliamoorthi wrote: > On 2014/10/29 18:14:56, stevenjb wrote: > > One line per member initializer (google_clang_format is your friend) > > I use clang format already and it formatted it this way. I verified the coding > guideline as well, it says if the entire list can fit in a line, it is okay. > Otherwise put it in seperate line. > > If you still insist, I can make this change in the end. As I am modifying the > code, I would rely on clang format and it would put it in a single line again, > making me repeat the work. Yeah, no, that's a semi recent change with clang-format that I'd forgotten, this is fine. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... chromeos/dbus/fake_shill_third_party_vpn_driver_client.cc:27: LOG(ERROR) << "Observer exists."; We should either allow this and use VLOG here, or CHECK to disallow it. Otherwise this has the potential to generate a bunch of ignored ERROR spam. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:20: ~FakeShillThirdPartyVpnDriverClient(); override https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:44: // ShillThirdPartyVpnDriverClient https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:116: sizeof(shill::kSetParametersKeyList[0]); Ugh, clang format did this? Maybe use a tmp for sizeof() / sizeof() (or, use arraysize() in base/macros.h). https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:141: } This pattern is used in several places, consider wrapping it in a helper method, e.g. helper_info = GetHelperInfo(object_path.value()); if (!helper_info) return; https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:205: it.value().GetAsString(&value)) { Invert logic, LOG(WARNING), and continue https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.h:42: virtual ~ShillThirdPartyVpnDriverClient(); ~ShillThirdPartyVpnDriverClient() override;
Patchset #3 (id:60001) has been deleted
PTAL. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... chromeos/dbus/fake_shill_third_party_vpn_driver_client.cc:27: LOG(ERROR) << "Observer exists."; On 2014/10/30 20:55:36, stevenjb wrote: > We should either allow this and use VLOG here, or CHECK to disallow it. > Otherwise this has the potential to generate a bunch of ignored ERROR spam. Done. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/fake_shill... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:20: ~FakeShillThirdPartyVpnDriverClient(); On 2014/10/30 20:55:36, stevenjb wrote: > override Done. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:44: On 2014/10/30 20:55:36, stevenjb wrote: > // ShillThirdPartyVpnDriverClient Done. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:116: sizeof(shill::kSetParametersKeyList[0]); On 2014/10/30 20:55:36, stevenjb wrote: > Ugh, clang format did this? Maybe use a tmp for sizeof() / sizeof() (or, use > arraysize() in base/macros.h). Done. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:141: } On 2014/10/30 20:55:36, stevenjb wrote: > This pattern is used in several places, consider wrapping it in a helper method, > e.g. helper_info = GetHelperInfo(object_path.value()); if (!helper_info) return; Done. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.cc:205: it.value().GetAsString(&value)) { On 2014/10/30 20:55:36, stevenjb wrote: > Invert logic, LOG(WARNING), and continue Done. https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/40001/chromeos/dbus/shill_thir... chromeos/dbus/shill_third_party_vpn_driver_client.h:42: virtual ~ShillThirdPartyVpnDriverClient(); On 2014/10/30 20:55:36, stevenjb wrote: > ~ShillThirdPartyVpnDriverClient() override; Done.
I added unit tests for the new client, it is complete now. PTAL.
Looking good, just a couple of comments. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_man... File chromeos/dbus/shill_manager_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_man... chromeos/dbus/shill_manager_client_unittest.cc:279: base::Bind(&ExpectDictionaryValueArgument, arg.get(), false), comment 'false' here and elsewhere https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_ser... File chromeos/dbus/shill_service_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_ser... chromeos/dbus/shill_service_client_unittest.cc:149: base::Bind(&ExpectDictionaryValueArgument, arg.get(), false), comment false https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:17: // TODO(kaliamoorthi): Move these constants to dbus/service_constants.cc The individual constants will be in service_constants.h, but there is no service_constants.cc, this should be in an anonymous namespace in this file. (We will need to maintain this array in Chrome, i.e. if an entry gets added to service_constants.h, we will need to modify this array. Not ideal, but in practice we will need to update any extension api / documentation anyway, so not really a big deal). https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:145: ++iter) { nit: Use c++11 style iterators here and below: for (auto& iter : helpers_) https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:223: << it.value() << "."; nit: Maybe have two ifs with separate warnings. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:40: extern const char* kSetParametersKeyList[]; Don't expose this. See comment in .cc file. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:45: #define PACKET_SIZE 5 Avoid defines like this, use 'const int kPacketSize = 5;'
Fixed comments from Steven. Paul Please review the CL from a logic perspective. You would be a good reviewer for the client side code since you reviewed the DBus service code. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_man... File chromeos/dbus/shill_manager_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_man... chromeos/dbus/shill_manager_client_unittest.cc:279: base::Bind(&ExpectDictionaryValueArgument, arg.get(), false), On 2014/10/31 16:35:41, stevenjb wrote: > comment 'false' here and elsewhere Done. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_ser... File chromeos/dbus/shill_service_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_ser... chromeos/dbus/shill_service_client_unittest.cc:149: base::Bind(&ExpectDictionaryValueArgument, arg.get(), false), On 2014/10/31 16:35:41, stevenjb wrote: > comment false Done. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:17: // TODO(kaliamoorthi): Move these constants to dbus/service_constants.cc On 2014/10/31 16:35:41, stevenjb wrote: > The individual constants will be in service_constants.h, but there is no > service_constants.cc, this should be in an anonymous namespace in this file. (We > will need to maintain this array in Chrome, i.e. if an entry gets added to > service_constants.h, we will need to modify this array. Not ideal, but in > practice we will need to update any extension api / documentation anyway, so not > really a big deal). Done. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:145: ++iter) { On 2014/10/31 16:35:41, stevenjb wrote: > nit: Use c++11 style iterators here and below: > for (auto& iter : helpers_) Done. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:223: << it.value() << "."; On 2014/10/31 16:35:41, stevenjb wrote: > nit: Maybe have two ifs with separate warnings. Done. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:40: extern const char* kSetParametersKeyList[]; On 2014/10/31 16:35:42, stevenjb wrote: > Don't expose this. See comment in .cc file. Done. https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/100001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:45: #define PACKET_SIZE 5 On 2014/10/31 16:35:42, stevenjb wrote: > Avoid defines like this, use 'const int kPacketSize = 5;' Done.
Just nits at this point. https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_man... File chromeos/dbus/shill_manager_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_man... chromeos/dbus/shill_manager_client_unittest.cc:277: bool string_valued = false; const bool https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_man... chromeos/dbus/shill_manager_client_unittest.cc:304: bool string_valued = false; const https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_ser... File chromeos/dbus/shill_service_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_ser... chromeos/dbus/shill_service_client_unittest.cc:148: bool string_valued = false; const https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:218: } else if (!it.value().GetAsString(&value)) { no else after continue or return
PTAL https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_man... File chromeos/dbus/shill_manager_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_man... chromeos/dbus/shill_manager_client_unittest.cc:277: bool string_valued = false; On 2014/11/03 16:55:23, stevenjb wrote: > const bool Done. https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_man... chromeos/dbus/shill_manager_client_unittest.cc:304: bool string_valued = false; On 2014/11/03 16:55:23, stevenjb wrote: > const Done. https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_ser... File chromeos/dbus/shill_service_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_ser... chromeos/dbus/shill_service_client_unittest.cc:148: bool string_valued = false; On 2014/11/03 16:55:23, stevenjb wrote: > const Done. https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/120001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:218: } else if (!it.value().GetAsString(&value)) { On 2014/11/03 16:55:23, stevenjb wrote: > no else after continue or return Done.
kaliamoorthi@chromium.org changed reviewers: + pneubeck@chromium.org - pstew@chromium.org
I added Phillipp to review the logic. PTAL.
lgtm
Patchset #7 (id:160001) has been deleted
looked at shill_third_party_vpn_driver_client so far https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:8: #include "base/macros.h" nit: not required if included in the header (see comment there) https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:66: return NULL; nit: NULL -> nullptr , also everywhere else where you use NULL of pointer type https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:90: typedef std::map<std::string, HelperInfo*> HelperMap; typedef -> using https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:92: void ReleaseHelper(const dbus::ObjectPath& object_path); please make it consistent whether you define functions inline or not. considering the size of this, I'd suggest making nothing inline. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:92: void ReleaseHelper(const dbus::ObjectPath& object_path); 'Release' is misleading. This term is usually used if ownership is released but the object is not deleted. Please rename to Remove/Delete https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:102: // Returns the corresponding ShillClientHelper for the object_path. 'Returns or creates' https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:112: // Returns the corresponding HelperInfo for the object_path. 'Returns or creates' https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:122: helpers_.insert(HelperMap::value_type(object_path.value(), helper_info)); nit: using operator[] seems easier to read https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:140: for (uint32 i = 0; i < arraysize(kSetParametersKeyList); ++i) { nit: braces are not required https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:167: const_cast<dbus::ObjectProxy*>(helper_info->helper()->object_proxy()); please try to get rid of the const_cast. e.g. make a separate CL to remove the 'const' attribute from the object_proxy() function https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:192: ReleaseHelper(object_path); potential BUG! ReleaseHelper will remove the helper object and the contained ShillClientHelper helper_, even if there is still a callback to the same object_path running! Probably you should unify the ownership model with ShillClientHelper's reference counting. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:221: std::string value; nit: move to first usage https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:238: ->CallVoidMethodWithErrorCallback(&method_call, callback, error_callback); here you're creating a helper that you only delete in ~ShillThirdPartyVpnDriverClientImpl or if RemoveShillThirdPartyVpnObserver is used at some time for the same object_path. You could use SetReleasedCallback, or at a minimum document that you're creating a helper here and not deleting the helper when the callback returns. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:276: const uint8* data; please initialize all primitives explicitly, just to be double sure that stuff is deterministic https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:277: size_t length; ditto: initialize https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:290: uint32 platform_message; ditto: initialize https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:8: #include "base/basictypes.h" should be base/macros.h if it's for DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:13: #include "dbus/object_path.h" nit: forward declare ObjectPath instead of including this header https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:17: // TODO(kaliamoorthi): Move these constants to dbus/service_constants.h please roll the cros_system dependency at first, instead of committing this todo
some more comments, overall looks rather good. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:8: #include "base/basictypes.h" replace basictypes.h by macros.h if you only need a DISALLOW* macro in the whole CL. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:14: // A fake implementation of ShillThirdPartyVpnDriverClient. please add some documentation what fake behavior you're approximately implementing. I think this not easy to guess for this client. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:54: typedef std::map<std::string, ShillThirdPartyVpnObserver*> ObserverMap; typedef -> using please replace typedef in the whole CL https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... File chromeos/dbus/shill_client_unittest_base.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.cc:10: #include "chromeos/dbus/shill_third_party_vpn_driver_client.h" already included in the header https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.cc:246: const uint8* bytes; built-in types should be initialized (also mentioned in the style guide that it's preferred: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables) https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.cc:312: if (string_valued) { wouldn't it have the same effect if you just check variant_reader.GetDataType()? e.g.: if (string_valued) EXPECT_EQ(dbus::Message::STRING, variant_reader.GetDataType()); https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... File chromeos/dbus/shill_client_unittest_base.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:101: class MockShillThirdPartyVpnObserver : public ShillThirdPartyVpnObserver { can be moved to shill_third_party_vpn_driver_client_unittest.cc https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:127: void SendPlatformMessageSignal(dbus::Signal* signal); Why are these new functions/members part of the base class if it's only used by shill_third_party_vpn_driver_client_unittest.cc ? this class is already very unstructured and crowded. i think it would be better to move this and all VPNDriver specific parts to the test fixture of shill_third_party_vpn_driver_client_unittest.cc. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:227: void OnConnectToPacketReceieved( typo: Receieved -> Received https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:42: CHECK(false); try not to crash the process if the class-under-test is faulty. e.g. do an assert directly in the test, which might require a state variable. or go with EXPECT if it's sufficient. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:67: // Set expectations. this doesn't convey new information. maybe something like "Expect each signal to be triggered once." https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_observer.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:8: #include "base/basictypes.h" should be base/macros.h instead. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:16: // Ownership of |data| belongs to the caller, hence the contents should be why not just passing a vector<uint8> or a string instead? at least add a minimum of documentation of the purpose of these events. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:23: virtual ~ShillThirdPartyVpnObserver() {} Add also a private: DISALLOW_ASSIGN
https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:50: const uint8* data, like for the observer, you should consider using string/vector instead of plain arrays.
Philipp, This addresses your previous comments. PTAL. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:8: #include "base/basictypes.h" On 2014/11/11 13:09:35, pneubeck wrote: > replace basictypes.h by macros.h if you only need a DISALLOW* macro in the whole > CL. This was added for uint32 etc. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:54: typedef std::map<std::string, ShillThirdPartyVpnObserver*> ObserverMap; On 2014/11/11 13:09:35, pneubeck wrote: > typedef -> using > please replace typedef in the whole CL Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... File chromeos/dbus/shill_client_unittest_base.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.cc:10: #include "chromeos/dbus/shill_third_party_vpn_driver_client.h" On 2014/11/11 13:09:35, pneubeck wrote: > already included in the header Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.cc:246: const uint8* bytes; On 2014/11/11 13:09:35, pneubeck wrote: > built-in types should be initialized > (also mentioned in the style guide that it's preferred: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables) Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.cc:312: if (string_valued) { On 2014/11/11 13:09:35, pneubeck wrote: > wouldn't it have the same effect if you just check variant_reader.GetDataType()? > e.g.: > > if (string_valued) > EXPECT_EQ(dbus::Message::STRING, variant_reader.GetDataType()); No, the way the data is marshaled is different if it is a variant vs a basic type like string. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... File chromeos/dbus/shill_client_unittest_base.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:101: class MockShillThirdPartyVpnObserver : public ShillThirdPartyVpnObserver { On 2014/11/11 13:09:35, pneubeck wrote: > can be moved to shill_third_party_vpn_driver_client_unittest.cc This is inline with the existing client. I understand this is arguable, but there is no compelling reason to not have it here. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:127: void SendPlatformMessageSignal(dbus::Signal* signal); On 2014/11/11 13:09:35, pneubeck wrote: > Why are these new functions/members part of the base class if it's only used by > shill_third_party_vpn_driver_client_unittest.cc ? > > this class is already very unstructured and crowded. i think it would be better > to move this and all VPNDriver specific parts to the test fixture of > shill_third_party_vpn_driver_client_unittest.cc. This is inline with the existing client. I understand this is arguable, but there is no compelling reason to not have it here. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:227: void OnConnectToPacketReceieved( On 2014/11/11 13:09:35, pneubeck wrote: > typo: Receieved -> Received Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:8: #include "base/macros.h" On 2014/11/10 16:44:07, pneubeck wrote: > nit: not required if included in the header (see comment there) Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:66: return NULL; On 2014/11/10 16:44:07, pneubeck wrote: > nit: NULL -> nullptr , also everywhere else where you use NULL of pointer type Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:90: typedef std::map<std::string, HelperInfo*> HelperMap; On 2014/11/10 16:44:08, pneubeck wrote: > typedef -> using Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:92: void ReleaseHelper(const dbus::ObjectPath& object_path); On 2014/11/10 16:44:07, pneubeck wrote: > please make it consistent whether you define functions inline or not. > considering the size of this, I'd suggest making nothing inline. Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:92: void ReleaseHelper(const dbus::ObjectPath& object_path); On 2014/11/10 16:44:08, pneubeck wrote: > 'Release' is misleading. This term is usually used if ownership is released but > the object is not deleted. > Please rename to Remove/Delete Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:102: // Returns the corresponding ShillClientHelper for the object_path. On 2014/11/10 16:44:07, pneubeck wrote: > 'Returns or creates' Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:112: // Returns the corresponding HelperInfo for the object_path. On 2014/11/10 16:44:07, pneubeck wrote: > 'Returns or creates' Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:122: helpers_.insert(HelperMap::value_type(object_path.value(), helper_info)); On 2014/11/10 16:44:08, pneubeck wrote: > nit: using operator[] seems easier to read Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:140: for (uint32 i = 0; i < arraysize(kSetParametersKeyList); ++i) { On 2014/11/10 16:44:07, pneubeck wrote: > nit: braces are not required I guess this is a matter of style. I prefer it this way. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:167: const_cast<dbus::ObjectProxy*>(helper_info->helper()->object_proxy()); On 2014/11/10 16:44:07, pneubeck wrote: > please try to get rid of the const_cast. > e.g. make a separate CL to remove the 'const' attribute from the object_proxy() > function I agree that this needs to be done. However, I am not sure about the order, so I am introducing a TODO to fix this. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:192: ReleaseHelper(object_path); On 2014/11/10 16:44:08, pneubeck wrote: > potential BUG! > > ReleaseHelper will remove the helper object and the contained ShillClientHelper > helper_, even if there is still a callback to the same object_path running! > Probably you should unify the ownership model with ShillClientHelper's reference > counting. We discussed this. But thanks for the thorough review. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:221: std::string value; On 2014/11/10 16:44:07, pneubeck wrote: > nit: move to first usage Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:238: ->CallVoidMethodWithErrorCallback(&method_call, callback, error_callback); On 2014/11/10 16:44:07, pneubeck wrote: > here you're creating a helper that you only delete in > ~ShillThirdPartyVpnDriverClientImpl or if RemoveShillThirdPartyVpnObserver is > used at some time for the same object_path. > > You could use SetReleasedCallback, or at a minimum document that you're creating > a helper here and not deleting the helper when the callback returns. We discussed this. To many object creation an deletion is not necessary here. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:276: const uint8* data; On 2014/11/10 16:44:07, pneubeck wrote: > please initialize all primitives explicitly, just to be double sure that stuff > is deterministic Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:277: size_t length; On 2014/11/10 16:44:08, pneubeck wrote: > ditto: initialize Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:290: uint32 platform_message; On 2014/11/10 16:44:07, pneubeck wrote: > ditto: initialize Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:8: #include "base/basictypes.h" On 2014/11/10 16:44:08, pneubeck wrote: > should be base/macros.h if it's for DISALLOW_COPY_AND_ASSIGN This is included for uint32 etc. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:13: #include "dbus/object_path.h" On 2014/11/10 16:44:08, pneubeck wrote: > nit: forward declare ObjectPath instead of including this header This is necessary for a kind of dependency injection, the user of this header is in extension/* that cannot include the header. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:17: // TODO(kaliamoorthi): Move these constants to dbus/service_constants.h On 2014/11/10 16:44:08, pneubeck wrote: > please roll the cros_system dependency at first, instead of committing this todo Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:50: const uint8* data, On 2014/11/11 13:17:16, pneubeck wrote: > like for the observer, you should consider using string/vector instead of plain > arrays. Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:42: CHECK(false); On 2014/11/11 13:09:35, pneubeck wrote: > try not to crash the process if the class-under-test is faulty. > e.g. do an assert directly in the test, which might require a state variable. > or go with EXPECT if it's sufficient. Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:67: // Set expectations. On 2014/11/11 13:09:35, pneubeck wrote: > this doesn't convey new information. > maybe something like "Expect each signal to be triggered once." Done. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_observer.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:8: #include "base/basictypes.h" On 2014/11/11 13:09:35, pneubeck wrote: > should be base/macros.h > instead. This is for uint32 https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:16: // Ownership of |data| belongs to the caller, hence the contents should be On 2014/11/11 13:09:35, pneubeck wrote: > why not just passing a vector<uint8> or a string instead? > > at least add a minimum of documentation of the purpose of these events. As we discussed this is to reduce memcpy. I documented it. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:23: virtual ~ShillThirdPartyVpnObserver() {} On 2014/11/11 13:09:35, pneubeck wrote: > Add also a > private: > DISALLOW_ASSIGN Done.
https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:8: #include "base/basictypes.h" On 2014/11/11 14:58:32, kaliamoorthi wrote: > On 2014/11/11 13:09:35, pneubeck wrote: > > replace basictypes.h by macros.h if you only need a DISALLOW* macro in the > whole > > CL. > > This was added for uint32 etc. From basictypes.h : // DEPRECATED: Please use (u)int{8,16,32,64}_t instead (and include <stdint.h>).
Patchset #9 (id:220001) has been deleted
PTAL https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_third_party_vpn_driver_client.h:8: #include "base/basictypes.h" On 2014/11/11 16:34:48, pneubeck wrote: > On 2014/11/11 14:58:32, kaliamoorthi wrote: > > On 2014/11/11 13:09:35, pneubeck wrote: > > > replace basictypes.h by macros.h if you only need a DISALLOW* macro in the > > whole > > > CL. > > > > This was added for uint32 etc. > > From basictypes.h : > // DEPRECATED: Please use (u)int{8,16,32,64}_t instead (and include <stdint.h>). Done.
Patchset #10 (id:260001) has been deleted
Patchset #9 (id:240001) has been deleted
@Steven, please take another look at shill_client_unittest_base.h and whether you see any reason why ThirdPartyVPN stuff should be added there instead of to the shill_third_party_vpn_driver_client_unittest.cc. I don't see a reason for that and would prefer to not clutter that base class even more. Also locality would be improved if it's in the unittest.cc https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... File chromeos/dbus/shill_client_unittest_base.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:101: class MockShillThirdPartyVpnObserver : public ShillThirdPartyVpnObserver { On 2014/11/11 14:58:33, kaliamoorthi wrote: > On 2014/11/11 13:09:35, pneubeck wrote: > > can be moved to shill_third_party_vpn_driver_client_unittest.cc > > This is inline with the existing client. I understand this is arguable, but > there is no compelling reason to not have it here. IMO, you need a compelling reason to break locality and not the other way round. Handing off to Steven, who is the primary maintainer of this class, to decide. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:127: void SendPlatformMessageSignal(dbus::Signal* signal); On 2014/11/11 14:58:33, kaliamoorthi wrote: > On 2014/11/11 13:09:35, pneubeck wrote: > > Why are these new functions/members part of the base class if it's only used > by > > shill_third_party_vpn_driver_client_unittest.cc ? > > > > this class is already very unstructured and crowded. i think it would be > better > > to move this and all VPNDriver specific parts to the test fixture of > > shill_third_party_vpn_driver_client_unittest.cc. > > This is inline with the existing client. I understand this is arguable, but > there is no compelling reason to not have it here. Again, IMO, there is no reason why locality should be broken in this case. Up to Steven to decide what he prefers. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:66: return NULL; On 2014/11/11 14:58:33, kaliamoorthi wrote: > On 2014/11/10 16:44:07, pneubeck wrote: > > nit: NULL -> nullptr , also everywhere else where you use NULL of pointer type > > Done. not done? please run s/NULL/nullptr/ over your whole CL and change it where NULL is ptr. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:192: ReleaseHelper(object_path); On 2014/11/11 14:58:33, kaliamoorthi wrote: > On 2014/11/10 16:44:08, pneubeck wrote: > > potential BUG! > > > > ReleaseHelper will remove the helper object and the contained > ShillClientHelper > > helper_, even if there is still a callback to the same object_path running! > > Probably you should unify the ownership model with ShillClientHelper's > reference > > counting. > > We discussed this. But thanks for the thorough review. Consider that in the case that I explained, the callback from SetParameters will not be run anymore without good reason. This is still a bug, just not an as problematic one ('not calling a callback' vs. 'crash') https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.h:13: #include "dbus/object_path.h" On 2014/11/11 14:58:34, kaliamoorthi wrote: > On 2014/11/10 16:44:08, pneubeck wrote: > > nit: forward declare ObjectPath instead of including this header > > This is necessary for a kind of dependency injection, the user of this header is > in extension/* that cannot include the header. This doesn't sound right to me. You should only use what is allowed by the DEPS files. If you think that the DEPS file is not up-to-date, then change that instead of relying no indirect includes. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:67: // Set expectations. On 2014/11/11 14:58:34, kaliamoorthi wrote: > On 2014/11/11 13:09:35, pneubeck wrote: > > this doesn't convey new information. > > maybe something like "Expect each signal to be triggered once." > > Done. the same argument applies to nearly every other comment in this file. Please fix it in all places. Note that you don't need to comment everything. It only makes sense to put a comment where it provides additional information to the reader. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:69: class HelperInfo { HelperInfo does not seem to require private access to ShillThirdPartyVpnDriverClientImpl, thus you could move it out of this class. your choice. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:91: void DeleteHelper(const dbus::ObjectPath& object_path); it would improve readability a bit if you'd move this declaration to the other *Helper() functions. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:101: // Returns or creates the corresponding ShillClientHelper for the object_path. references to variables/arguments are usually put between '|'. here it would be |object_path|. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:42: EXPECT(false, true); should be EXPECT_FALSE(true) ? https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_observer.h (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:16: ShillThirdPartyVpnObserver() {} nit: you _could_ have dropped this constructor if you use DISALLOW_ASSIGN instead of DISALLOW_COPY_AND_ASSIGN. both is fine with me.
As it turns out, I both agree and disagree with what code should be in shill_client_unittest_base :) https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... File chromeos/dbus/shill_client_unittest_base.h (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:101: class MockShillThirdPartyVpnObserver : public ShillThirdPartyVpnObserver { On 2014/11/12 14:21:06, pneubeck wrote: > On 2014/11/11 14:58:33, kaliamoorthi wrote: > > On 2014/11/11 13:09:35, pneubeck wrote: > > > can be moved to shill_third_party_vpn_driver_client_unittest.cc > > > > This is inline with the existing client. I understand this is arguable, but > > there is no compelling reason to not have it here. > > IMO, you need a compelling reason to break locality and not the other way round. > > Handing off to Steven, who is the primary maintainer of this class, to decide. This observer doesn't belong here. MockPropertyChangeObserver is shared by multiple tests, this is not. You should be able to put it in an anonymous namespace in the unittest. https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_cli... chromeos/dbus/shill_client_unittest_base.h:127: void SendPlatformMessageSignal(dbus::Signal* signal); On 2014/11/12 14:21:06, pneubeck wrote: > On 2014/11/11 14:58:33, kaliamoorthi wrote: > > On 2014/11/11 13:09:35, pneubeck wrote: > > > Why are these new functions/members part of the base class if it's only used > > by > > > shill_third_party_vpn_driver_client_unittest.cc ? > > > > > > this class is already very unstructured and crowded. i think it would be > > better > > > to move this and all VPNDriver specific parts to the test fixture of > > > shill_third_party_vpn_driver_client_unittest.cc. > > > > This is inline with the existing client. I understand this is arguable, but > > there is no compelling reason to not have it here. > > Again, IMO, there is no reason why locality should be broken in this case. > > Up to Steven to decide what he prefers. This (and the other changes) are fine since they are not vpn specific and could be used by (and improved for) other tests. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:65: return NULL; s/NULL/nullptr throughout https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:71: explicit HelperInfo(dbus::ObjectProxy* object_proxy); Blank line between constructor and other methods https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:42: EXPECT(false, true); On 2014/11/12 14:21:07, pneubeck wrote: > should be EXPECT_FALSE(true) ? Also, << error_name to help debug. You might want to name this UnexpectedFailure, or pass a bound |expected| parameter. (Sometimes we include tests that expect a failure).
PTAL. This addresses all previous comments. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:65: return NULL; On 2014/11/12 18:05:13, stevenjb wrote: > s/NULL/nullptr throughout Missed it in previous CL. Done. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:69: class HelperInfo { On 2014/11/12 14:21:07, pneubeck wrote: > HelperInfo does not seem to require private access to > ShillThirdPartyVpnDriverClientImpl, thus you could move it out of this class. > > your choice. HelperInfo does not make sense outside the class, so I'd prefer it to be inside. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:71: explicit HelperInfo(dbus::ObjectProxy* object_proxy); On 2014/11/12 18:05:13, stevenjb wrote: > Blank line between constructor and other methods Done. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:91: void DeleteHelper(const dbus::ObjectPath& object_path); On 2014/11/12 14:21:07, pneubeck wrote: > it would improve readability a bit if you'd move this declaration to the other > *Helper() functions. Done. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:101: // Returns or creates the corresponding ShillClientHelper for the object_path. On 2014/11/12 14:21:07, pneubeck wrote: > references to variables/arguments are usually put between '|'. > here it would be |object_path|. Done. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:42: EXPECT(false, true); On 2014/11/12 18:05:13, stevenjb wrote: > On 2014/11/12 14:21:07, pneubeck wrote: > > should be EXPECT_FALSE(true) ? > Also, << error_name to help debug. > You might want to name this UnexpectedFailure, or pass a bound |expected| > parameter. (Sometimes we include tests that expect a failure). Done. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:42: EXPECT(false, true); On 2014/11/12 14:21:07, pneubeck wrote: > should be EXPECT_FALSE(true) ? Done. https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_observer.h (right): https://codereview.chromium.org/681723003/diff/280001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_observer.h:16: ShillThirdPartyVpnObserver() {} On 2014/11/12 14:21:07, pneubeck wrote: > nit: > you _could_ have dropped this constructor if you use DISALLOW_ASSIGN instead of > DISALLOW_COPY_AND_ASSIGN. > both is fine with me. Acknowledged.
bartfab@chromium.org changed reviewers: + bartfab@chromium.org
https://codereview.chromium.org/681723003/diff/300001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/300001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:50: EXPECT_FALSE(true) << error_name << error_message; Instead of EXPECT_FALSE(true), you should use ADD_FAILURE().
https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/681723003/diff/180001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client.cc:192: ReleaseHelper(object_path); On 2014/11/12 14:21:07, pneubeck wrote: > On 2014/11/11 14:58:33, kaliamoorthi wrote: > > On 2014/11/10 16:44:08, pneubeck wrote: > > > potential BUG! > > > > > > ReleaseHelper will remove the helper object and the contained > > ShillClientHelper > > > helper_, even if there is still a callback to the same object_path running! > > > Probably you should unify the ownership model with ShillClientHelper's > > reference > > > counting. > > > > We discussed this. But thanks for the thorough review. > > Consider that in the case that I explained, the callback from SetParameters will > not be run anymore without good reason. > This is still a bug, just not an as problematic one ('not calling a callback' > vs. 'crash') Note that as long as this is not clarified this CL shouldn't land. There are several solutions. Either - add a clarifying comment and ideally a unit test that ensures that the mentioned case works - change the implementation, so that it's obvious that it works - give examples that show that this usage/pattern is very common (I didn't find any from a quick search, e.g. other ShillServiceClient does _not_ do this) Note that the latter is very controversial, as the code should be understandable to unfamiliar developers. https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:22: ~MockShillThirdPartyVpnObserver() {} misses an override (didn't this complain at compilation or git cl upload?) https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:35: virtual void SetUp() { virtual -> override https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:45: virtual void TearDown() { ShillClientUnittestBase::TearDown(); } virtual -> override
PTAL. I added a test to ensure that the callbacks get fired even after the helper is destroyed. https://codereview.chromium.org/681723003/diff/300001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/300001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:50: EXPECT_FALSE(true) << error_name << error_message; On 2014/11/13 12:18:47, bartfab wrote: > Instead of EXPECT_FALSE(true), you should use ADD_FAILURE(). Acknowledged. https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:22: ~MockShillThirdPartyVpnObserver() {} On 2014/11/13 14:06:11, pneubeck wrote: > misses an override > (didn't this complain at compilation or git cl upload?) Done. https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:35: virtual void SetUp() { On 2014/11/13 14:06:11, pneubeck wrote: > virtual -> override Done. https://codereview.chromium.org/681723003/diff/320001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:45: virtual void TearDown() { ShillClientUnittestBase::TearDown(); } On 2014/11/13 14:06:11, pneubeck wrote: > virtual -> override Done.
https://codereview.chromium.org/681723003/diff/340001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/340001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:84: SendPacketReceievedSignal(&preceived_signal); ah, I missed before that you have to run the message loop here and verify and clear the expectations: https://code.google.com/p/googlemock/wiki/CheatSheet#Verifying_and_Resetting_... https://codereview.chromium.org/681723003/diff/340001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:86: EXPECT_CALL(*this, MockSuccess()).Times(1); you should set this expectation after you called RemoveShillThirdPartyVpnObserver
https://codereview.chromium.org/681723003/diff/340001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/340001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:84: SendPacketReceievedSignal(&preceived_signal); On 2014/11/13 16:40:12, pneubeck wrote: > ah, I missed before that you have to run the message loop here and verify and > clear the expectations: > https://code.google.com/p/googlemock/wiki/CheatSheet#Verifying_and_Resetting_... Done. https://codereview.chromium.org/681723003/diff/340001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:86: EXPECT_CALL(*this, MockSuccess()).Times(1); On 2014/11/13 16:40:12, pneubeck wrote: > you should set this expectation after you called > RemoveShillThirdPartyVpnObserver Done.
lgtm
On 2014/11/13 17:14:25, pneubeck wrote: > lgtm Thanks Philipp. Steven, Can you please give me an owner lgtm?
owner lgtm https://codereview.chromium.org/681723003/diff/360001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/360001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:50: ADD_FAILURE() << error_name << error_message; nit: Separate error name and message, e.g. with ": ".
On 2014/11/13 17:53:56, stevenjb wrote: > owner lgtm > > https://codereview.chromium.org/681723003/diff/360001/chromeos/dbus/shill_thi... > File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): > > https://codereview.chromium.org/681723003/diff/360001/chromeos/dbus/shill_thi... > chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:50: ADD_FAILURE() > << error_name << error_message; > nit: Separate error name and message, e.g. with ": ". Thanks Steven.
https://codereview.chromium.org/681723003/diff/360001/chromeos/dbus/shill_thi... File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right): https://codereview.chromium.org/681723003/diff/360001/chromeos/dbus/shill_thi... chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:50: ADD_FAILURE() << error_name << error_message; On 2014/11/13 17:53:56, stevenjb wrote: > nit: Separate error name and message, e.g. with ": ". Done.
The CQ bit was checked by kaliamoorthi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/681723003/380001
Message was sent while issue was closed.
Committed patchset #14 (id:380001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/4c839047c821a5109fd8b5b828f1ca2082cca07a Cr-Commit-Position: refs/heads/master@{#304054} |
