Refactor native_backend_kwallet_x

chrome/browser/password_manager/kwallet_dbus.h

Index: chrome/browser/password_manager/kwallet_dbus.h
diff --git a/chrome/browser/password_manager/kwallet_dbus.h b/chrome/browser/password_manager/kwallet_dbus.h
new file mode 100644
index 0000000000000000000000000000000000000000..ca6d9f15d0243b5f9c946303d1550da250e470d5
--- /dev/null
+++ b/chrome/browser/password_manager/kwallet_dbus.h
@@ -0,0 +1,114 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PASSWORD_MANAGER_KWALLET_DBUS_H_
+#define CHROME_BROWSER_PASSWORD_MANAGER_KWALLET_DBUS_H_
+
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/scoped_vector.h"

[vasilii, 2016/06/10 16:58:09]

Used?

[cfroussios, 2016/06/16 12:29:08]

Done.

+#include "base/nix/xdg_util.h"
+#include "dbus/bus.h"

[vasilii, 2016/06/10 16:58:09]

Used?

[cfroussios, 2016/06/16 12:29:08]

For GetBus() and SetSessionBus()

[vasilii, 2016/06/16 17:54:43]

Can be forward declared.

[cfroussios, 2016/06/17 12:17:26]

Done.

+

[vasilii, 2016/06/10 16:58:10]

A comment.

[cfroussios, 2016/06/16 12:29:08]

Done.

+class KWalletDBus {
+ public:
+  // Error code for dbus calls to kwallet.
+  enum Error { SUCCESS = 0, CANNOT_CONTACT, CANNOT_READ };

[vasilii, 2016/06/10 16:58:09]

What about enum class?

[vasilii, 2016/06/10 16:58:09]

split one per line; Optionally remove 0. It's by default 0.

[cfroussios, 2016/06/16 12:29:08]

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.

[cfroussios, 2016/06/16 12:29:08]

An enum class complicated error handling code, because we can't do
KWalletDBus::Error error = ...
if (error)

+
+  explicit KWalletDBus(base::nix::DesktopEnvironment desktop_env);
+  virtual ~KWalletDBus();

[vasilii, 2016/06/10 16:58:09]

Is there any reason for 'virtual'?

[cfroussios, 2016/06/16 12:29:08]

Done.

+
+  // Set the bus that we should use. Required before any other operation.
+  void SetSessionBus(scoped_refptr<dbus::Bus> session_bus);
+
+  // Expose the bus so that shutdown can be scheduled asynchronously
+  dbus::Bus* GetBus();

[vasilii, 2016/06/10 16:58:09]

Name consistently with SetSessionBus. GetSessionBus?

[cfroussios, 2016/06/16 12:29:08]

Done.

+
+  // Use KLauncher to start the KWallet service. Returns true if successful.
+  bool StartKWalletd();
+
+  // The functions below are wrappers for calling the eponymous KWallet dbus
+  // methods. They take pointers to locations where the return values will be
+  // written. More KWallet documentation at
+  // https://api.kde.org/4.12-api/kdelibs-apidocs/kdeui/html/classKWallet_1_1Wallet.html
+
+  // Determine if the KDE wallet is enabled.
+  Error IsEnabled(bool* enabled);
+
+  // The name of the wallet used to store network passwords.

[vasilii, 2016/06/10 16:58:09]

Sets the name or gets the name?

[cfroussios, 2016/06/16 12:29:08]

Done.

+  Error NetworkWallet(std::string* wallet_name_ptr);
+
+  // Determine if the current folder has they entry key.
+  Error HasEntry(bool* has_entry_ptr,

[vasilii, 2016/06/10 16:58:10]

Here and below: the out parameters should be the last ones.

[cfroussios, 2016/06/16 12:29:08]

Done.

+                 const int wallet_handle,

[vasilii, 2016/06/10 16:58:09]

const int and const size_t don't really make sense

[cfroussios, 2016/06/16 12:29:08]

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.

[vasilii, 2016/06/16 17:54:43]

You could have 'const' in .cc only. In the header it doesn't really matter.

[cfroussios, 2016/06/17 12:17:26]

Done.

+                 const std::string& folder_name,
+                 const std::string& signon_realm,
+                 const std::string& app_name);
+
+  // Read the entry key from the current folder.
+  Error ReadEntry(const uint8_t** bytes_ptr,

[vasilii, 2016/06/10 16:58:10]

You can apply WARN_UNUSED_RESULT to all the methods returning Error.

[cfroussios, 2016/06/16 12:29:08]

Done.

+                  size_t* length_ptr,
+                  const int wallet_handle,
+                  const std::string& folder_name,
+                  const std::string& signon_realm,
+                  const std::string& app_name);
+
+  // Return the list of keys of all entries in this folder.
+  Error EntryList(std::vector<std::string>* entry_list_ptr,
+                  const int wallet_handle,
+                  const std::string& folder_name,
+                  const std::string& app_name);
+
+  // Remove the entry key from the current folder.
+  Error RemoveEntry(int* rv_ptr,
+                    const int wallet_handle,
+                    const std::string& folder_name,
+                    const std::string& signon_realm,
+                    const std::string& app_name);
+
+  // Write a binary entry to the current folder.
+  Error WriteEntry(int* rv_ptr,

[vasilii, 2016/06/10 16:58:09]

What's this?

[cfroussios, 2016/06/16 12:29:08]

Done.

+                   const int wallet_handle,
+                   const std::string& folder_name,
+                   const std::string& signon_realm,
+                   const uint8_t* data,
+                   const size_t length,
+                   const std::string& app_name);
+
+  // Open the |wallet_name| wallet for use.
+  Error Open(int* handle_ptr,
+             const std::string& wallet_name,
+             const std::string& app_name);
+
+  // Determine if the folder |folder_name| exists in the wallet.
+  Error HasFolder(bool* has_folder_ptr,
+                  const int handle,
+                  const std::string& folder_name,
+                  const std::string& app_name);
+
+  // Created the folder |fodler_name|.
+  Error CreateFolder(bool* success_ptr,
+                     const int handle,
+                     const std::string& folder_name,
+                     const std::string& app_name);
+
+ private:
+  // DBus handle for communication with klauncher and kwalletd.
+  scoped_refptr<dbus::Bus> session_bus_;
+  // Object proxy for kwalletd. We do not own this.
+  dbus::ObjectProxy* kwallet_proxy_;
+
+  // KWallet DBus name
+  std::string dbus_service_name_;
+  // DBus path to KWallet interfaces
+  std::string dbus_path_;
+  // The name used for logging and by klauncher when starting KWallet
+  std::string kwalletd_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(KWalletDBus);
+};
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_KWALLET_DBUS_H_