[Chromoting] GetHostAttributes with tests

remoting/host/host_attributes_win.h

+// 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_ATTRIBUTES_WIN_H_
+#define REMOTING_HOST_HOST_ATTRIBUTES_WIN_H_
+
+#include "remoting/host/host_attributes.h"
+#include "third_party/webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h"
+
+namespace remoting {
+
+namespace {
+
+inline constexpr bool IsDebug() {

[Sergey Ulanov, 2016/11/07 20:11:33]

I don't think that putting this in the per-platform header is the right thing to
do. E.g. here you had to duplicate this function. Instead it's better to have a
single list and ifdef any platform-specific values.

[Hzj_jie, 2016/11/08 01:29:54]

Done.

+#if defined(NDEBUG)
+  return false;
+#else
+  return true;
+#endif
+}
+
+}  // namespace
+
+// TODO(zijiehe): Add DirectX version attributes. Blocked by change
+// https://codereview.chromium.org/2468083002/.
+static StaticAttribute kStaticAttributes[] = {

[Hzj_jie, 2016/11/05 05:29:28]

MSVC does not allow to change the value of a constexpr (neither mutable nor
const_cast), it simply crashes. So this array is not constexpr.

+  StaticAttribute::Create( { "Debug-Build", &IsDebug } ),

[Hzj_jie, 2016/11/05 05:29:28]

MSVC does not allow sizeof(int[0]), i.e. size of an empty array, so at least we
need to have one StaticAttribute in this array.

[Sergey Ulanov, 2016/11/07 20:11:33]

StaticAttribute::Create() is not constexpr, so this code will require static
initializer, which is not allowed. See
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/p6h3HC8Wro4
You can easily avoid it by not calling a function here.

[Hzj_jie, 2016/11/08 01:29:54]

I think requiring static initializer or not depends more on whether the object
is POD and whether the input parameters are compiling time determinable, instead
of whether the function is constexpr. Surely constexpr functions calls in static
objects are always compile time initialized, but non-constexpr function calls
are not necessarily required static initialization. If the input parameters
(DynamicAttribute in my case) are compiling time determinable, compiler will be
smart enough to optimize if optimization enabled. And nm proves my opinion.
-----
(Release)
zijiehe@zijiehe - Mon Nov 07-16:37:19:~/chromium/chromium2/src$ nm
out/GNRelease/remoting_unittests | grep _GLOBAL | grep host_attributes.cc

(Debug)
zijiehe@zijiehe - Mon Nov 07-16:37:27:~/chromium/chromium2/src$ nm
out/GNDebug/remoting_unittests | grep _GLOBAL | grep host_attributes.cc
000000000049e580 t _GLOBAL__sub_I_host_attributes.cc
-----
But correct me if I am wrong.

The reason I use a DynamicAttribute in a StaticAttribute is because of the
concern of evaluating time. Evaluating an attribute is usually complex than
directly returning true or false. I believe "Debug-Build" is the only attribute
we can evaluate during compiling time.
Using OS version, which should be a StaticAttribute since it won't change during
the lifetime of the application, as an example. We usually need to call an OS
API to get the version, and caching its result is valuable. Note, the name of
the attributes are static per POD requirement, we will need to call this OS
version API for several times to fill all the fields. Say, {"OS-WIN-7",
&WhetherWeAreOnWin7}, {"OS-WIN-8", &WhetherWeAreOnWin8}, {"OS-WIN-8.1",
&WhetherWeAreOnWin8_1}, {"OS-WIN-10", &WhetherWeAreOnWin10}, {"OS-WIN-10.1",
&WhetherWeAreOnWin10_1}.

[Sergey Ulanov, 2016/11/08 20:11:05]

I think if an attributes cannot be evaluated in compile time, then is shouldn't
be called static. You essentially want to cache values for dynamic attributes
that never change, but is that really necessary? Most functions in base::SysInfo
already cache results when it's appropriate, so I don't think we really need to
cache them here. Maybe make all attributes dynamic to simplify this code?

[Hzj_jie, 2016/11/09 02:37:52]

Yes, that's a good opinion, for expensive but cacheable function calls, they
should cache the value themselves anyway.

+};
+static constexpr DynamicAttribute kDynamicAttributes[] = {
+  { "DirectX-Capturer", &webrtc::ScreenCapturerWinDirectx::IsSupported },
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_HOST_ATTRIBUTES_WIN_H_