Changed location of LibsecretLoader

components/os_crypt/libsecret_loader.h

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[vabr (Chromium), 2016/04/28 07:53:21]

nit: No (c), see
http://dev.chromium.org/developers/coding-style#TOC-File-headers .

[cfroussios, 2016/04/28 12:18:11]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OS_CRYPT_LIBSECRET_LOADER_H_
+#define COMPONENTS_OS_CRYPT_LIBSECRET_LOADER_H_
+
+#include <libsecret/secret.h>
+
+#include <string>
+
+#include "base/macros.h"
+
+class LibsecretLoader {
+ public:
+  static decltype(&::secret_password_store_sync) secret_password_store_sync;
+  static decltype(&::secret_service_search_sync) secret_service_search_sync;
+  static decltype(&::secret_password_clear_sync) secret_password_clear_sync;
+  static decltype(&::secret_item_get_secret) secret_item_get_secret;
+  static decltype(&::secret_value_get_text) secret_value_get_text;
+  static decltype(&::secret_item_get_attributes) secret_item_get_attributes;
+  static decltype(&::secret_item_load_secret_sync) secret_item_load_secret_sync;
+  static decltype(&::secret_value_unref) secret_value_unref;
+
+ protected:
+  static bool LoadLibsecret();
+  static bool LibsecretIsAvailable();
+
+  static bool libsecret_loaded;
+
+ private:
+  struct FunctionInfo {
+    const char* name;
+    void** pointer;
+  };
+
+  static const FunctionInfo functions[];
+};

[vabr (Chromium), 2016/04/28 07:53:21]

Please add
DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader);

as the very last line in the private section, separated by a blank line above.

(The purpose of the macro is to prevent creating instances of the class. This is
an all-static class, so creating an instance would typically mean a typo in the
code.)

[cfroussios, 2016/04/28 12:18:11]

NativeBackendLibsecret inherits from LibsecretLoader and therefore
LibsecretLoader needs to be instantiated.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...

I'm adding DISALLOW_COPY_AND_ASSIGN instead. 

[vabr (Chromium), 2016/04/28 12:44:25]

It does not really make sense to derive from a static-only class, including the
questionable benefits of the protected visibility. When we are already cleaning
this up, I suggest:
(1) Make LoadLibsecret() and IsLibsecretAvailable() public.
(2) Make libsecret_loaded private.
(3) Change NativeBackendLibsecret to stop inheriting from LibsecretLoader, and
add LibsecretLoader:: to the calls in NativeBackendLibsecret::Init.
(4) Apply DISALLOW_IMPLICIT_CONSTRUCTORS to LibsecretLoader and delete the
default constructor.

[vabr (Chromium), 2016/04/28 14:42:12]

Ping.

[cfroussios, 2016/04/28 16:26:57]

The libsecret field needs to stay protected, so that it is mockable for tests.

[cfroussios, 2016/04/28 16:26:57]

Done.

[vabr (Chromium), 2016/04/29 07:36:45]

Acknowledged.

+
+#endif  // COMPONENTS_OS_CRYPT_LIBSECRET_LOADER_H_