Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2057123002:
Refactor native_backend_kwallet_x (Closed)
|
Created:
4 years, 6 months ago by cfroussios Modified:
4 years, 6 months ago Reviewers:
vasilii CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor native_backend_kwallet_x
Parts of the implementation have been moved into a new util, KWalletDBus.
The purpose of this is to make those pieces of code reusable, so that the
(upcoming) KWallet implementation of OSCrypt can use them and avoid code
duplication.
BUG=602624
Committed: https://crrev.com/890de0dd1b69237f36aaeab4955a7357e30c404e
Cr-Commit-Position: refs/heads/master@{#401245}
Patch Set 1 #Patch Set 2 : doc #Patch Set 3 : cleanup #
Total comments: 48
Patch Set 4 : Recommendations #Patch Set 5 : nit #Patch Set 6 : Tests #Patch Set 7 : nit #
Total comments: 36
Patch Set 8 : Recommendations #Patch Set 9 : cleanup #
Total comments: 10
Patch Set 10 : feedback #Patch Set 11 : consistency #
Total comments: 3
Patch Set 12 : assert #Patch Set 13 : revert assertions #
Messages
Total messages: 20 (6 generated)
Patchset #3 (id:40001) has been deleted
cfroussios@chromium.org changed reviewers: + vasilii@chromium.org
Description was changed from ========== Refactor native_backend_kwallet_x Parts of the implementation have been moved into a new util, KWalletDBus. The purpose of this is to make those pieces of code reusable, so that the (upcoming) KWallet implementation of OSCrypt can use them and avoid code duplication. BUG= ========== to ========== Refactor native_backend_kwallet_x Parts of the implementation have been moved into a new util, KWalletDBus. The purpose of this is to make those pieces of code reusable, so that the (upcoming) KWallet implementation of OSCrypt can use them and avoid code duplication. BUG=602624 ==========
What about unit tests for KWalletDBus? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/kwallet_dbus.cc (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.cc:43: session_bus_->ShutdownAndBlock(); Why do you kill it here and in NativeBackendKWallet::~NativeBackendKWallet()? Also threading isn't clear. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.cc:180: dbus::MessageReader reader(response.get()); It's destroyed here and |bytes| becomes invalid. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/kwallet_dbus.h (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:12: #include "base/memory/scoped_vector.h" Used? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:14: #include "dbus/bus.h" Used? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:15: A comment. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:19: enum Error { SUCCESS = 0, CANNOT_CONTACT, CANNOT_READ }; split one per line; Optionally remove 0. It's by default 0. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:19: enum Error { SUCCESS = 0, CANNOT_CONTACT, CANNOT_READ }; What about enum class? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:22: virtual ~KWalletDBus(); Is there any reason for 'virtual'? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:28: dbus::Bus* GetBus(); Name consistently with SetSessionBus. GetSessionBus? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:41: // The name of the wallet used to store network passwords. Sets the name or gets the name? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:45: Error HasEntry(bool* has_entry_ptr, Here and below: the out parameters should be the last ones. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:46: const int wallet_handle, const int and const size_t don't really make sense https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:52: Error ReadEntry(const uint8_t** bytes_ptr, You can apply WARN_UNUSED_RESULT to all the methods returning Error. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:73: Error WriteEntry(int* rv_ptr, What's this? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:367: // klauncher to start it. Obsolete. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:377: bool enabled; init please https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:378: error = kwallet_dbus_.IsEnabled(&enabled); KWalletDBus::Error error = ... https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:390: if (error == KWalletDBus::Error::CANNOT_READ) switch is more appropriate here and above. If you add more errors then the compiler won't be happy that it's forgotten here. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:583: base::Pickle pickle(reinterpret_cast<const char*>(bytes), length); Would static_cast works here? https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:674: // TODO(cfroussios) how does this make sense? kInvalidKWalletHandle == -1 nonsense. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:691: return kInvalidKWalletHandle; nonsense again https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:806: { what is the point of those {} here and before?
Hi. I added tests for KWalletDBus https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/kwallet_dbus.cc (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.cc:43: session_bus_->ShutdownAndBlock(); On 2016/06/10 16:58:09, vasilii wrote: > Why do you kill it here and in NativeBackendKWallet::~NativeBackendKWallet()? > Also threading isn't clear. Done. The responsibility of destroying the bus was moved to the owner of KWalletDBus, but this was left behind. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.cc:180: dbus::MessageReader reader(response.get()); On 2016/06/10 16:58:09, vasilii wrote: > It's destroyed here and |bytes| becomes invalid. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/kwallet_dbus.h (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:12: #include "base/memory/scoped_vector.h" On 2016/06/10 16:58:09, vasilii wrote: > Used? Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:14: #include "dbus/bus.h" On 2016/06/10 16:58:09, vasilii wrote: > Used? For GetBus() and SetSessionBus() https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:15: On 2016/06/10 16:58:10, vasilii wrote: > A comment. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:19: enum Error { SUCCESS = 0, CANNOT_CONTACT, CANNOT_READ }; On 2016/06/10 16:58:09, vasilii wrote: > What about enum class? An enum class complicated error handling code, because we can't do KWalletDBus::Error error = ... if (error) https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:19: enum Error { SUCCESS = 0, CANNOT_CONTACT, CANNOT_READ }; On 2016/06/10 16:58:09, vasilii wrote: > split one per line; Optionally remove 0. It's by default 0. The style was chosen by the formatter. Success = 0 emphasizes that there is code which depends on the fact that success is 0 and errors are not. It stops trivially reordering SUCCESS. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:22: virtual ~KWalletDBus(); On 2016/06/10 16:58:09, vasilii wrote: > Is there any reason for 'virtual'? Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:28: dbus::Bus* GetBus(); On 2016/06/10 16:58:09, vasilii wrote: > Name consistently with SetSessionBus. GetSessionBus? Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:41: // The name of the wallet used to store network passwords. On 2016/06/10 16:58:09, vasilii wrote: > Sets the name or gets the name? Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:45: Error HasEntry(bool* has_entry_ptr, On 2016/06/10 16:58:10, vasilii wrote: > Here and below: the out parameters should be the last ones. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:46: const int wallet_handle, On 2016/06/10 16:58:09, vasilii wrote: > const int and const size_t don't really make sense const means that it's a value and not a variable. The code is small enough that I can just promise to myself that I will use them as values, but I like to call values out wherever c++ gives me a reasonable tool to do so. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:52: Error ReadEntry(const uint8_t** bytes_ptr, On 2016/06/10 16:58:10, vasilii wrote: > You can apply WARN_UNUSED_RESULT to all the methods returning Error. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:73: Error WriteEntry(int* rv_ptr, On 2016/06/10 16:58:09, vasilii wrote: > What's this? Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:367: // klauncher to start it. On 2016/06/10 16:58:10, vasilii wrote: > Obsolete. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:377: bool enabled; On 2016/06/10 16:58:10, vasilii wrote: > init please Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:378: error = kwallet_dbus_.IsEnabled(&enabled); On 2016/06/10 16:58:10, vasilii wrote: > KWalletDBus::Error error = ... Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:390: if (error == KWalletDBus::Error::CANNOT_READ) On 2016/06/10 16:58:10, vasilii wrote: > switch is more appropriate here and above. If you add more errors then the > compiler won't be happy that it's forgotten here. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:583: base::Pickle pickle(reinterpret_cast<const char*>(bytes), length); On 2016/06/10 16:58:10, vasilii wrote: > Would static_cast works here? Compile-time error. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:674: // TODO(cfroussios) how does this make sense? kInvalidKWalletHandle == -1 On 2016/06/10 16:58:10, vasilii wrote: > nonsense. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:691: return kInvalidKWalletHandle; On 2016/06/10 16:58:10, vasilii wrote: > nonsense again Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/native_backend_kwallet_x.cc:806: { On 2016/06/10 16:58:10, vasilii wrote: > what is the point of those {} here and before? The previous author preferred scoping reappearing variables I think. Now that it's just one error code, they don't make much sense so I removed them.
https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/kwallet_dbus.h (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:14: #include "dbus/bus.h" On 2016/06/16 12:29:08, cfroussios wrote: > On 2016/06/10 16:58:09, vasilii wrote: > > Used? > > For GetBus() and SetSessionBus() Can be forward declared. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:46: const int wallet_handle, On 2016/06/16 12:29:08, cfroussios wrote: > On 2016/06/10 16:58:09, vasilii wrote: > > const int and const size_t don't really make sense > > const means that it's a value and not a variable. The code is small enough that > I can just promise to myself that I will use them as values, but I like to call > values out wherever c++ gives me a reasonable tool to do so. You could have 'const' in .cc only. In the header it doesn't really matter. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.cc:178: size_t length = -1; Why is so strange value? https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus.h (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.h:53: // Ownership of the instance assigned to |bytes_ptr| is passed to the caller Isn't it obvious? https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.h:85: Error Open(const std::string& wallet_name, Probably should go to the top of the header. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:52: dbus::Response* RespondArrayOfStrings(std::vector<std::string> strings) { const std::vector<std::string>& https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:85: void SetUp() override { Why not doing everything in the constructor? https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:103: // The klauncher proxy is aquired every time StartKWalletd() is tested It's tested only once. Therefore, the EXPECT_CALL should go there. Instead of WillRepeatedly it becomes WillOnce. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:104: EXPECT_CALL( EXPECT_CALLs should be just above the method which triggers them. That one isn't called in SetUp(). Ideally you call testing::Mock::VerifyAndClearExpectations() in the end of SetUp(). Otherwise, if I create an empty test it will fail, that's strange. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:112: EXPECT_CALL(*mock_kwallet_proxy_.get(), MockCallMethodAndBlock(_, _)) This is achieved by instantiating StrictMock<dbus::MockObjectProxy> instead of dbus::MockObjectProxy. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:116: virtual ~KWalletDBusTest() = default; That's probably unnecessary. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:119: base::nix::DesktopEnvironment desktop_env_; const? https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:144: ACTION_P3(PopIntStringString, int_1, str_2, str_3) { This is not an action. This is a matcher because you test some properties of the argument. MATCHER_P(MessageHasInt, int_val, , "") { reader.PopInt32(&i); return int_val == i; } MATCHER_P(MessageHasString, str_val, , "") { reader.PopString(&str); return str == str_val; } In the test: EXPECT_CALL(*mock_kwallet_proxy_.get(), MockCallMethodAndBlock(AllOf( Calls(kKWalletInterface, "hasEntry"), MessageHasInt(7), MessageHasString("asdf"), MessageHasString("vbnm")), _)) .WillOnce(Return(RespondBool(true))); https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:266: .WillOnce(Return(nullptr)); Here and below. Those WillOnce are extremely confusing. It should be EXPECT_CALL().WillOnce(Return(somethins)); EXPECT_TRUE(kwallet_dbus_.StartKWalletd()); EXPECT_CALL().WillOnce(Return(something_else)); EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); Ideally, they are even in the different tests. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_kwallet_x.cc:649: if (bytes.size() == 0 || In the line 570 you don't check it. There should be consistency. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_kwallet_x.cc:814: KWalletDBus::Error error = Reuse previous |error|?
https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/kwallet_dbus.h (right): https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:14: #include "dbus/bus.h" On 2016/06/16 17:54:43, vasilii wrote: > On 2016/06/16 12:29:08, cfroussios wrote: > > On 2016/06/10 16:58:09, vasilii wrote: > > > Used? > > > > For GetBus() and SetSessionBus() > > Can be forward declared. Done. https://codereview.chromium.org/2057123002/diff/60001/chrome/browser/password... chrome/browser/password_manager/kwallet_dbus.h:46: const int wallet_handle, On 2016/06/16 17:54:43, vasilii wrote: > On 2016/06/16 12:29:08, cfroussios wrote: > > On 2016/06/10 16:58:09, vasilii wrote: > > > const int and const size_t don't really make sense > > > > const means that it's a value and not a variable. The code is small enough > that > > I can just promise to myself that I will use them as values, but I like to > call > > values out wherever c++ gives me a reasonable tool to do so. > > You could have 'const' in .cc only. In the header it doesn't really matter. Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.cc:178: size_t length = -1; On 2016/06/16 17:54:43, vasilii wrote: > Why is so strange value? I initialize to invalid values to make more visible (i.e. more likely to fail) the case, where the value is used without being set by the code, which is actually supposed to set it. Do you think this is too confusing compared to the benefit? https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus.h (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.h:53: // Ownership of the instance assigned to |bytes_ptr| is passed to the caller On 2016/06/16 17:54:43, vasilii wrote: > Isn't it obvious? Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.h:85: Error Open(const std::string& wallet_name, On 2016/06/16 17:54:43, vasilii wrote: > Probably should go to the top of the header. Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:52: dbus::Response* RespondArrayOfStrings(std::vector<std::string> strings) { On 2016/06/16 17:54:43, vasilii wrote: > const std::vector<std::string>& Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:85: void SetUp() override { On 2016/06/16 17:54:44, vasilii wrote: > Why not doing everything in the constructor? Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:103: // The klauncher proxy is aquired every time StartKWalletd() is tested On 2016/06/16 17:54:43, vasilii wrote: > It's tested only once. Therefore, the EXPECT_CALL should go there. Instead of > WillRepeatedly it becomes WillOnce. Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:104: EXPECT_CALL( On 2016/06/16 17:54:43, vasilii wrote: > EXPECT_CALLs should be just above the method which triggers them. That one isn't > called in SetUp(). > Ideally you call testing::Mock::VerifyAndClearExpectations() in the end of > SetUp(). Otherwise, if I create an empty test it will fail, that's strange. Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:112: EXPECT_CALL(*mock_kwallet_proxy_.get(), MockCallMethodAndBlock(_, _)) On 2016/06/16 17:54:44, vasilii wrote: > This is achieved by instantiating StrictMock<dbus::MockObjectProxy> instead of > dbus::MockObjectProxy. I cannot instantiate StrictMock<dbus::MockObjectProxy>, because MockObjectProxy doesn't have a public destructor. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:116: virtual ~KWalletDBusTest() = default; On 2016/06/16 17:54:43, vasilii wrote: > That's probably unnecessary. Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:119: base::nix::DesktopEnvironment desktop_env_; On 2016/06/16 17:54:43, vasilii wrote: > const? Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:144: ACTION_P3(PopIntStringString, int_1, str_2, str_3) { On 2016/06/16 17:54:43, vasilii wrote: > This is not an action. This is a matcher because you test some properties of the > argument. > > MATCHER_P(MessageHasInt, int_val, , "") { > reader.PopInt32(&i); > return int_val == i; > } > > MATCHER_P(MessageHasString, str_val, , "") { > reader.PopString(&str); > return str == str_val; > } > > In the test: > > EXPECT_CALL(*mock_kwallet_proxy_.get(), > MockCallMethodAndBlock(AllOf( > Calls(kKWalletInterface, "hasEntry"), > MessageHasInt(7), > MessageHasString("asdf"), > MessageHasString("vbnm")), _)) > .WillOnce(Return(RespondBool(true))); I like the style of composing the expectation one argument at a time. I couldn't make it work without making things even more complicated, because they have to share a reader, which has to be created during/after the call. Also, in theory, a matcher should not have side effects, such as advancing the read pointer. In general, if a matcher fails, you try matching against the next pattern until you find something. I do not know if gmock would actually do that, but that's how I understand the concept of matchers. Actions, on the other hand, can modify state. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:266: .WillOnce(Return(nullptr)); On 2016/06/16 17:54:44, vasilii wrote: > Here and below. Those WillOnce are extremely confusing. It should be > > EXPECT_CALL().WillOnce(Return(somethins)); > EXPECT_TRUE(kwallet_dbus_.StartKWalletd()); > > EXPECT_CALL().WillOnce(Return(something_else)); > EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); > > Ideally, they are even in the different tests. According to https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md "you mustn't interleave EXPECT_CALL()s and calls to the mock functions" The documentation for gmock is full of examples about preparing for multiple calls, and then performing those calls. It shouldn't be confusing. I've moved checking the error codes to separate tests, if that helps. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_kwallet_x.cc:649: if (bytes.size() == 0 || On 2016/06/16 17:54:44, vasilii wrote: > In the line 570 you don't check it. There should be consistency. Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_kwallet_x.cc:814: KWalletDBus::Error error = On 2016/06/16 17:54:44, vasilii wrote: > Reuse previous |error|? Done.
https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.cc:178: size_t length = -1; On 2016/06/17 12:17:26, cfroussios wrote: > On 2016/06/16 17:54:43, vasilii wrote: > > Why is so strange value? > > I initialize to invalid values to make more visible (i.e. more likely to fail) > the case, where the value is used without being set by the code, which is > actually supposed to set it. Do you think this is too confusing compared to the > benefit? We don't want to fail more likely ;-) Also we don't want to include tests to the production code. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:112: EXPECT_CALL(*mock_kwallet_proxy_.get(), MockCallMethodAndBlock(_, _)) On 2016/06/17 12:17:27, cfroussios wrote: > On 2016/06/16 17:54:44, vasilii wrote: > > This is achieved by instantiating StrictMock<dbus::MockObjectProxy> instead of > > dbus::MockObjectProxy. > > I cannot instantiate StrictMock<dbus::MockObjectProxy>, because MockObjectProxy > doesn't have a public destructor. It's protected. What is the problem? scoped_refptr<dbus::MockObjectProxy> mock_klauncher_proxy = new StrictMock<MockObjectProxy>(...); https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:144: ACTION_P3(PopIntStringString, int_1, str_2, str_3) { On 2016/06/17 12:17:26, cfroussios wrote: > On 2016/06/16 17:54:43, vasilii wrote: > > This is not an action. This is a matcher because you test some properties of > the > > argument. > > > > MATCHER_P(MessageHasInt, int_val, , "") { > > reader.PopInt32(&i); > > return int_val == i; > > } > > > > MATCHER_P(MessageHasString, str_val, , "") { > > reader.PopString(&str); > > return str == str_val; > > } > > > > In the test: > > > > EXPECT_CALL(*mock_kwallet_proxy_.get(), > > MockCallMethodAndBlock(AllOf( > > Calls(kKWalletInterface, "hasEntry"), > > MessageHasInt(7), > > MessageHasString("asdf"), > > MessageHasString("vbnm")), _)) > > .WillOnce(Return(RespondBool(true))); > > I like the style of composing the expectation one argument at a time. I couldn't > make it work without making things even more complicated, because they have to > share a reader, which has to be created during/after the call. > > Also, in theory, a matcher should not have side effects, such as advancing the > read pointer. In general, if a matcher fails, you try matching against the next > pattern until you find something. I do not know if gmock would actually do that, > but that's how I understand the concept of matchers. Actions, on the other hand, > can modify state. Good point regarding the reader. This should work: EXPECT_CALL(*mock_kwallet_proxy_.get(), MockCallMethodAndBlock(AllOf( Calls(kKWalletInterface, "hasEntry"), ArgumentsAre(7, "asdf", "vbnm")), _)) .WillOnce(Return(RespondBool(true))); https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:266: .WillOnce(Return(nullptr)); On 2016/06/17 12:17:27, cfroussios wrote: > On 2016/06/16 17:54:44, vasilii wrote: > > Here and below. Those WillOnce are extremely confusing. It should be > > > > EXPECT_CALL().WillOnce(Return(somethins)); > > EXPECT_TRUE(kwallet_dbus_.StartKWalletd()); > > > > EXPECT_CALL().WillOnce(Return(something_else)); > > EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); > > > > Ideally, they are even in the different tests. > > According to > https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md > "you mustn't interleave EXPECT_CALL()s and calls to the mock functions" > > The documentation for gmock is full of examples about preparing for multiple > calls, and then performing those calls. It shouldn't be confusing. > > I've moved checking the error codes to separate tests, if that helps. That's why I mentioned different tests (or VerifyAndClearExpectation). Most of the committers will agree that EXPECT_TRUE(kwallet_dbus_.StartKWalletd()); EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); is weird. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:45: dbus::Response* RespondBytes(const uint8_t* bytes, int count) { Why not passing const::vector here? https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:241: dbus::Response* response_success = dbus::Response::CreateEmpty().release(); RespondEmpty()? https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:248: const std::vector<std::string> empty; I don't think it makes test more readable. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:278: .WillOnce(Return(nullptr)); You definitely merged two tests together here. There are only 46 occurrences of .WillOnce().WillOnce in chrome https://cs.chromium.org/search/?q=pcre:yes++WillOnce%5C(.*%5C)%5Cs*%5C.WillOn... Only few of them test the same function twice. It's a bad pattern. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_kwallet_x.cc:570: if (bytes.size() && !bytes.empty()
https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus.cc:178: size_t length = -1; On 2016/06/17 14:43:40, vasilii wrote: > On 2016/06/17 12:17:26, cfroussios wrote: > > On 2016/06/16 17:54:43, vasilii wrote: > > > Why is so strange value? > > > > I initialize to invalid values to make more visible (i.e. more likely to fail) > > the case, where the value is used without being set by the code, which is > > actually supposed to set it. Do you think this is too confusing compared to > the > > benefit? > > We don't want to fail more likely ;-) Also we don't want to include tests to the > production code. Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:112: EXPECT_CALL(*mock_kwallet_proxy_.get(), MockCallMethodAndBlock(_, _)) On 2016/06/17 14:43:41, vasilii wrote: > On 2016/06/17 12:17:27, cfroussios wrote: > > On 2016/06/16 17:54:44, vasilii wrote: > > > This is achieved by instantiating StrictMock<dbus::MockObjectProxy> instead > of > > > dbus::MockObjectProxy. > > > > I cannot instantiate StrictMock<dbus::MockObjectProxy>, because > MockObjectProxy > > doesn't have a public destructor. > > It's protected. What is the problem? > > scoped_refptr<dbus::MockObjectProxy> mock_klauncher_proxy = new > StrictMock<MockObjectProxy>(...); > Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:144: ACTION_P3(PopIntStringString, int_1, str_2, str_3) { On 2016/06/17 14:43:40, vasilii wrote: > On 2016/06/17 12:17:26, cfroussios wrote: > > On 2016/06/16 17:54:43, vasilii wrote: > > > This is not an action. This is a matcher because you test some properties of > > the > > > argument. > > > > > > MATCHER_P(MessageHasInt, int_val, , "") { > > > reader.PopInt32(&i); > > > return int_val == i; > > > } > > > > > > MATCHER_P(MessageHasString, str_val, , "") { > > > reader.PopString(&str); > > > return str == str_val; > > > } > > > > > > In the test: > > > > > > EXPECT_CALL(*mock_kwallet_proxy_.get(), > > > MockCallMethodAndBlock(AllOf( > > > Calls(kKWalletInterface, "hasEntry"), > > > MessageHasInt(7), > > > MessageHasString("asdf"), > > > MessageHasString("vbnm")), _)) > > > .WillOnce(Return(RespondBool(true))); > > > > I like the style of composing the expectation one argument at a time. I > couldn't > > make it work without making things even more complicated, because they have to > > share a reader, which has to be created during/after the call. > > > > Also, in theory, a matcher should not have side effects, such as advancing the > > read pointer. In general, if a matcher fails, you try matching against the > next > > pattern until you find something. I do not know if gmock would actually do > that, > > but that's how I understand the concept of matchers. Actions, on the other > hand, > > can modify state. > > Good point regarding the reader. This should work: > > EXPECT_CALL(*mock_kwallet_proxy_.get(), > MockCallMethodAndBlock(AllOf( > Calls(kKWalletInterface, "hasEntry"), > ArgumentsAre(7, "asdf", "vbnm")), _)) > .WillOnce(Return(RespondBool(true))); Done. https://codereview.chromium.org/2057123002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:266: .WillOnce(Return(nullptr)); On 2016/06/17 14:43:40, vasilii wrote: > On 2016/06/17 12:17:27, cfroussios wrote: > > On 2016/06/16 17:54:44, vasilii wrote: > > > Here and below. Those WillOnce are extremely confusing. It should be > > > > > > EXPECT_CALL().WillOnce(Return(somethins)); > > > EXPECT_TRUE(kwallet_dbus_.StartKWalletd()); > > > > > > EXPECT_CALL().WillOnce(Return(something_else)); > > > EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); > > > > > > Ideally, they are even in the different tests. > > > > According to > > https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md > > "you mustn't interleave EXPECT_CALL()s and calls to the mock functions" > > > > The documentation for gmock is full of examples about preparing for multiple > > calls, and then performing those calls. It shouldn't be confusing. > > > > I've moved checking the error codes to separate tests, if that helps. > > That's why I mentioned different tests (or VerifyAndClearExpectation). Most of > the committers will agree that > > EXPECT_TRUE(kwallet_dbus_.StartKWalletd()); > EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); > EXPECT_FALSE(kwallet_dbus_.StartKWalletd()); > > is weird. Done. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:45: dbus::Response* RespondBytes(const uint8_t* bytes, int count) { On 2016/06/17 14:43:41, vasilii wrote: > Why not passing const::vector here? Done. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:241: dbus::Response* response_success = dbus::Response::CreateEmpty().release(); On 2016/06/17 14:43:41, vasilii wrote: > RespondEmpty()? Done. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:248: const std::vector<std::string> empty; On 2016/06/17 14:43:41, vasilii wrote: > I don't think it makes test more readable. Done. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:278: .WillOnce(Return(nullptr)); On 2016/06/17 14:43:41, vasilii wrote: > You definitely merged two tests together here. There are only 46 occurrences of > .WillOnce().WillOnce in chrome > > https://cs.chromium.org/search/?q=pcre:yes++WillOnce%5C(.*%5C)%5Cs*%5C.WillOn... > > Only few of them test the same function twice. It's a bad pattern. Done. https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): https://codereview.chromium.org/2057123002/diff/180001/chrome/browser/passwor... chrome/browser/password_manager/native_backend_kwallet_x.cc:570: if (bytes.size() && On 2016/06/17 14:43:41, vasilii wrote: > !bytes.empty() Done.
lgtm I like the tests! https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:138: EXPECT_TRUE(reader.PopInt32(&i)); What about ASSERT_TRUE here?
https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:138: EXPECT_TRUE(reader.PopInt32(&i)); On 2016/06/21 16:25:22, vasilii wrote: > What about ASSERT_TRUE here? Done.
https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... chrome/browser/password_manager/kwallet_dbus_unittest.cc:138: EXPECT_TRUE(reader.PopInt32(&i)); On 2016/06/21 17:01:42, cfroussios wrote: > On 2016/06/21 16:25:22, vasilii wrote: > > What about ASSERT_TRUE here? > > Done. Turns out this won't work. ASSERT_TRUE terminates a test by returning void. A matcher returns a boolean and therefore it doesn't compile.
On 2016/06/22 09:20:07, cfroussios wrote: > https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... > File chrome/browser/password_manager/kwallet_dbus_unittest.cc (right): > > https://codereview.chromium.org/2057123002/diff/220001/chrome/browser/passwor... > chrome/browser/password_manager/kwallet_dbus_unittest.cc:138: > EXPECT_TRUE(reader.PopInt32(&i)); > On 2016/06/21 17:01:42, cfroussios wrote: > > On 2016/06/21 16:25:22, vasilii wrote: > > > What about ASSERT_TRUE here? > > > > Done. > > Turns out this won't work. ASSERT_TRUE terminates a test by returning void. A > matcher returns a boolean and therefore it doesn't compile. Oh, the same problem like in the constructors. LGTM
The CQ bit was checked by cfroussios@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2057123002/260001
Message was sent while issue was closed.
Description was changed from ========== Refactor native_backend_kwallet_x Parts of the implementation have been moved into a new util, KWalletDBus. The purpose of this is to make those pieces of code reusable, so that the (upcoming) KWallet implementation of OSCrypt can use them and avoid code duplication. BUG=602624 ========== to ========== Refactor native_backend_kwallet_x Parts of the implementation have been moved into a new util, KWalletDBus. The purpose of this is to make those pieces of code reusable, so that the (upcoming) KWallet implementation of OSCrypt can use them and avoid code duplication. BUG=602624 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Refactor native_backend_kwallet_x Parts of the implementation have been moved into a new util, KWalletDBus. The purpose of this is to make those pieces of code reusable, so that the (upcoming) KWallet implementation of OSCrypt can use them and avoid code duplication. BUG=602624 ========== to ========== Refactor native_backend_kwallet_x Parts of the implementation have been moved into a new util, KWalletDBus. The purpose of this is to make those pieces of code reusable, so that the (upcoming) KWallet implementation of OSCrypt can use them and avoid code duplication. BUG=602624 Committed: https://crrev.com/890de0dd1b69237f36aaeab4955a7357e30c404e Cr-Commit-Position: refs/heads/master@{#401245} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/890de0dd1b69237f36aaeab4955a7357e30c404e Cr-Commit-Position: refs/heads/master@{#401245} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
