Changed location of LibsecretLoader

components/os_crypt/os_crypt_posix_util_libsecret.h

Index: components/os_crypt/os_crypt_posix_util_libsecret.h
diff --git a/components/os_crypt/os_crypt_posix_util_libsecret.h b/components/os_crypt/os_crypt_posix_util_libsecret.h
new file mode 100644
index 0000000000000000000000000000000000000000..32fbaaed66e87f45bb12306d1150bbca95e82c6c
--- /dev/null
+++ b/components/os_crypt/os_crypt_posix_util_libsecret.h
@@ -0,0 +1,65 @@
+// 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 COMPONENTS_OS_CRYPT_OS_CRYPT_POSIX_UTIL_LIBSECRET_H_

[Lei Zhang, 2016/04/28 21:49:09]

Can we name the file libsecret_util_posix.h?

[cfroussios, 2016/04/29 12:14:05]

Done.

+#define COMPONENTS_OS_CRYPT_OS_CRYPT_POSIX_UTIL_LIBSECRET_H_
+
+#include <libsecret/secret.h>
+
+#include <list>
+#include <string>
+
+#include "base/macros.h"
+
+class LibsecretLoader {

[Lei Zhang, 2016/04/28 21:49:09]

Can you add some documentation to explain what this class does?

I'm guessing one must call LibsecretIsAvailable() to see if the library exists.
Then LoadLibsecret() to initialize the function pointers.

[cfroussios, 2016/04/29 12:14:05]

I understand the confusion. I've introduced a method that does things in the
correct order and made the two previous methods private. I've also added
documentation.

+ 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;
+
+  static bool LoadLibsecret();
+
+  static bool LibsecretIsAvailable();
+
+ protected:
+  static bool libsecret_loaded;
+
+ private:
+  struct FunctionInfo {
+    const char* name;
+    void** pointer;
+  };
+
+  static const FunctionInfo functions[];
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(LibsecretLoader);
+};
+
+class LibsecretAttributesBuilder {
+ public:
+  LibsecretAttributesBuilder();
+  ~LibsecretAttributesBuilder();
+
+  void Append(const std::string& name, const std::string& value);
+
+  void Append(const std::string& name, int64_t value);
+
+  // GHashTable, its keys and values returned from Get() are destroyed in
+  // |LibsecretAttributesBuilder| desctructor.

[Lei Zhang, 2016/04/28 21:49:09]

typo

[cfroussios, 2016/04/29 12:14:05]

Done.

+  GHashTable* Get() { return attrs_; }
+
+ private:
+  // |name_values_| is a storage for strings referenced in |attrs_|.
+  std::list<std::string> name_values_;

[Lei Zhang, 2016/04/28 21:49:09]

I realize this is code being moved, but what's the advantage of having a list,
vs just using a vector?

[cfroussios, 2016/04/29 12:14:05]

There are no benefits that I can see. I've switched to a vector

[cfroussios, 2016/04/29 16:39:37]

It turns out that list is necessary. Switching to vector is what caused the asan
bot to keep failing. The failures go away if this is a list or if we call a
sufficient vector.reserve before appending attributes.

name_values_ serves as storage for others to refer to. My hypothesis is that
those references can be broken if the vector moves things while changing its
capacity. In contract, list nodes will stay where they are.

I'm keeping it as a list for now. It might make sense in the future to explore
alternative ways of doing the same thing without depending on the internals of
containers.

+  GHashTable* attrs_;
+
+  DISALLOW_COPY_AND_ASSIGN(LibsecretAttributesBuilder);
+};
+
+#endif  // COMPONENTS_OS_CRYPT_OS_CRYPT_POSIX_UTIL_LIBSECRET_H_