[Chromoting] GetHostAttributes with tests

remoting/host/host_traits.cc

Index: remoting/host/host_traits.cc
diff --git a/remoting/host/host_traits.cc b/remoting/host/host_traits.cc
new file mode 100644
index 0000000000000000000000000000000000000000..55e15fac3a337859ee5094c078618145db726ab2
--- /dev/null
+++ b/remoting/host/host_traits.cc
@@ -0,0 +1,84 @@
+// 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.
+
+#include "remoting/host/host_traits.h"
+
+#include "base/logging.h"
+#include "base/strings/string_piece.h"
+#include "remoting/host/directx_traits.h"
+
+namespace remoting {
+
+namespace {
+static constexpr char kSeparator[] = ",";
+}  // namespace
+
+HostTraits& host_traits() {
+  // This instance does not release to ensure it can still work during the
+  // ending of the process.
+  static HostTraits* instance = new HostTraits();

[Sergey Ulanov, 2016/11/02 20:10:32]

base::Singleton should be used for singletons. Chromium is always compiled with
disabled thread-safe initialization for statics.

But I also don't see why this needs to be a singleton. I'd prefer to have it
allocated when the host is initialized and then passed around as necessary.

[Hzj_jie, 2016/11/03 02:29:34]

I think this is a typical scenario of singleton. Only one instance of this class
is needed during the lifetime of the binary runtime.

[Sergey Ulanov, 2016/11/03 19:05:34]

There are many other classes for which we have only one instance per process,
yet they are not singletons. 
Here is a good discussion on why singletons are to be avoided:
https://www.michaelsafyan.com/tech/design/patterns/singleton

1. If you envision this class as dynamic (as it currently is), i.e. the content
can be change depending on host configuration, then it shouldn't be a singleton
(e.g. in case we run multiple host instances in one process).
2. If it's intended as static list of attributes that never changes for the same
version of the host, then there is no point allocating lists of values
dynamically and it can be just static list of features, as I suggested in my
other comment.

[Hzj_jie, 2016/11/03 21:33:30]

Done.

+  return *instance;
+}
+
+HostTraits::HostTraits() {
+  RegisterDirectxTraits();
+}
+
+HostTraits::~HostTraits() = default;
+
+std::string HostTraits::ToString() const {
+  std::string result;
+  for (const auto& str : static_values_) {
+    DCHECK(!str.empty());
+    if (!result.empty()) {
+      result.append(kSeparator);
+    }
+    result.append(str);
+  }
+
+  for (const auto& pair : dynamic_values_) {
+    DCHECK(!pair.first.empty());
+    DCHECK(!pair.second.is_null());
+    if (pair.second.Run()) {
+      if (!result.empty()) {
+        result.append(kSeparator);
+      }
+      result.append(pair.first);
+    }
+  }
+
+  return result;
+}
+
+void HostTraits::Register(const std::string& key) {
+  DCHECK(key.find(kSeparator) == std::string::npos);
+  DCHECK(dynamic_values_.find(key) == dynamic_values_.end());
+  DCHECK(static_values_.find(key) == static_values_.end());
+  static_values_.insert(key);
+}
+
+void HostTraits::Register(const std::string& key,
+                          const base::Callback<bool(void)>& value) {
+  DCHECK(key.find(kSeparator) == std::string::npos);
+  DCHECK(dynamic_values_.find(key) == dynamic_values_.end());
+  DCHECK(static_values_.find(key) == static_values_.end());
+  dynamic_values_[key] = value;
+}
+
+void HostTraits::Erase(const std::string& key) {
+  size_t removed = dynamic_values_.erase(key) + static_values_.erase(key);
+  DCHECK(removed == 1);
+}
+
+// static
+bool HostTraits::ContainsTrait(const std::string& output,
+                               const std::string& key) {
+  return output == key ||
+         base::StringPiece(output).ends_with(std::string(kSeparator) + key) ||
+         base::StringPiece(output).starts_with(key + kSeparator) ||
+         output.find(std::string(kSeparator) + key + kSeparator)
+             != std::string::npos;
+}
+
+}  // namespace remoting