[Chromoting] GetHostAttributes with tests

remoting/host/host_traits.h

Index: remoting/host/host_traits.h
diff --git a/remoting/host/host_traits.h b/remoting/host/host_traits.h
new file mode 100644
index 0000000000000000000000000000000000000000..8e1dac2f8e5babf8c5bdb3823d9301fb5624a31c
--- /dev/null
+++ b/remoting/host/host_traits.h
@@ -0,0 +1,52 @@
+// 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 REMOTING_HOST_HOST_TRAITS_H_
+#define REMOTING_HOST_HOST_TRAITS_H_
+
+#include <map>
+#include <set>
+#include <string>
+
+#include "base/callback.h"
+
+namespace remoting {
+
+class HostTraits& host_traits();
+
+class HostTraits final {

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

Normally term "trait" means something different, see
https://en.wikipedia.org/wiki/Trait_(computer_programming) .
So I suggest to call this something else, maybe attributes.

But overall this looks like it's more complex than necessary. Can it be replaced
with just a static list of all supported attributes, e.g.:

struct HostAttributes {
  void name;
  bool(*get_function)();
};

constexpr HostAttributes g_dynamic_attributes[] = {
 { "directx_capturer", &IsDirectxCapturerSupported };
}

Is there any reason this would not work here?

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

Then we will lose the functionality of ToString(), which will convert all the
attributes into its text representation, and send from host to client.

If we add this function as a free function, it will become a class in struct +
function format.

Also, not all the attributes are dynamic evaluated, say OS family, they won't
change during the lifetime of the binary.

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

Not sure why that would be a problem. Just to make it clear, HostAttributes
struct and all values can be hidden in host_traits.cc. host_traits.h only needs
to define function(s) to get list of attributes, e.g.:

std::string GetAttributesAsString();

We can always make it more complicated in future when necessary.
That's fine. We could have static list for them:

struct HostStaticAttributes {
  char* name;
  char* value;
};

constexpr HostStaticAttributes g_static_attributes[] = {
 { "os", OS },
};

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

Acknowledged.

+ public:
+  ~HostTraits();
+
+  // Converts the HostTraits into its text representation, which is a comma-
+  // separated string, each field indicates one trait.
+  std::string ToString() const;
+
+  // Registers a static value, which will be appended to ToString() result.
+  void Register(const std::string& key);
+
+  // Registers a dynamic value, which will be evaluated each time ToString() is
+  // called.
+  void Register(const std::string& key,
+                const base::Callback<bool(void)>& value);

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

don't need void.

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

Done.

+
+  // Erases a value. This function is usually for testing purpose.
+  void Erase(const std::string& key);

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

Do you really need this? Not sure we will ever want to erase traits.

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

This is for test only, in HostTraitsTest.DynamicValuesShouldBeEvaluated, it will
erase the fake trait after it finishes.

Do you prefer to change it into a private function, and create friend
relationship with HostTraitsTest?

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

That's actually one of the reasons why singletons are anti-pattern: they are
hard to test. It wouldn't be a problem if this class wasn't a singleton.

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

Done.

+
+  // Whether the |output| contains the specified trait as |key|. This function
+  // is usually for testing purpose.
+  static bool ContainsTrait(const std::string& output, const std::string& key);
+
+ private:
+  // A singleton object.
+  friend HostTraits& host_traits();
+  HostTraits();
+
+  std::set<std::string> static_values_;
+  std::map<std::string, base::Callback<bool(void)>> dynamic_values_;

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

I don't think you really need set or map here. vector<> would be more
appropriate. There will be only a handful of elements and you'll never need to
lookup by name.

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

The set and map are used to ensure no keys are duplicate. Considering this class
generates pure text, a duplicate key will eventually break the logic in client
side or impact experiment selection.

+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_HOST_TRAITS_H_