[Chromoting] GetHostAttributes with tests

[Chromoting] GetHostAttributes with tests

This change adds GetHostAttributes function, which returns a comma-separated
string to represent all attributes of the host, into host. Host sends the result
of this function to client to indicate the capability of host experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926
Committed: https://crrev.com/3010c8d9f1cb51a31f1dd22c42fa68b7eeacf27a
Cr-Commit-Position: refs/heads/master@{#431451}

[Hzj_jie, 2016-11-02 00:19:23]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 00:20:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 01:27:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 01:27:02]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2016-11-02 19:17:33]

Description was changed from

==========
HostTraits with tests

BUG=650926
==========

to

==========
[Chromoting] HostTraits with tests

This change adds HostTraits, which is a singleton object to represent all traits
of the host, into host. Host uses this component to build a comma-separated
string, and send to client to indicate the capable of host experiments. The
design of HostTraits can be found at http://shortn/_J7i5IOexlO.

This is part of Chromoting Host Experiment feature, overall design doc is @
http://shortn/_7q3ylJYRD3.

BUG=650926
==========

[Hzj_jie, 2016-11-02 19:17:34]

zijiehe@chromium.org changed reviewers:
+ jamiewalch@chromium.org, sergeyu@chromium.org

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

https://codereview.chromium.org/2464293002/diff/1/remoting/host/directx_traits.h
File remoting/host/directx_traits.h (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/directx_trait...
remoting/host/directx_traits.h:4: 
Add include guards.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.cc
File remoting/host/host_traits.cc (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.c...
remoting/host/host_traits.cc:20: static HostTraits* instance = new HostTraits();
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.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h
File remoting/host/host_traits.h (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:18: class HostTraits final {
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?

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:32: const base::Callback<bool(void)>& value);
don't need void.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:35: void Erase(const std::string& key);
Do you really need this? Not sure we will ever want to erase traits.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:47: std::map<std::string,
base::Callback<bool(void)>> dynamic_values_;
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 00:08:13]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 00:08:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 01:27:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 01:27:31]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2464293002/diff/1/remoting/host/directx_traits.h
File remoting/host/directx_traits.h (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/directx_trait...
remoting/host/directx_traits.h:4: 
On 2016/11/02 20:10:31, Sergey Ulanov wrote:
> Add include guards.

Done.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.cc
File remoting/host/host_traits.cc (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.c...
remoting/host/host_traits.cc:20: static HostTraits* instance = new HostTraits();
On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> 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.

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

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h
File remoting/host/host_traits.h (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:18: class HostTraits final {
On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> 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?

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.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:32: const base::Callback<bool(void)>& value);
On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> don't need void.

Done.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:35: void Erase(const std::string& key);
On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> Do you really need this? Not sure we will ever want to erase traits.

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?

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:47: std::map<std::string,
base::Callback<bool(void)>> dynamic_values_;
On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> 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.

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.

[Hzj_jie, 2016-11-03 03:19:13]

Description was changed from

==========
[Chromoting] HostTraits with tests

This change adds HostTraits, which is a singleton object to represent all traits
of the host, into host. Host uses this component to build a comma-separated
string, and send to client to indicate the capable of host experiments. The
design of HostTraits can be found at http://shortn/_J7i5IOexlO.

This is part of Chromoting Host Experiment feature, overall design doc is @
http://shortn/_7q3ylJYRD3.

BUG=650926
==========

to

==========
[Chromoting] HostAttributes with tests

This change adds HostAttributes, which is a singleton object to represent all
attributes of the host, into host. Host uses this component to build a
comma-separated string, and send to client to indicate the capability of host
experiments. The design of HostAttributes can be found at
http://shortn/_J7i5IOexlO.

This is part of Chromoting Host Experiment feature, overall design doc is @
http://shortn/_7q3ylJYRD3.

BUG=650926
==========

[Hzj_jie, 2016-11-03 03:20:32]

Looks like private reviews in codereview.chromium.org are discouraged. Since
this change will eventually be public, I removed protected flag of this change.
But all the documents are still for Google internal only.

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

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.cc
File remoting/host/host_traits.cc (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.c...
remoting/host/host_traits.cc:20: static HostTraits* instance = new HostTraits();
On 2016/11/03 02:29:34, Hzj_jie wrote:
> On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> > 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.
> 
> I think this is a typical scenario of singleton. Only one instance of this
class
> is needed during the lifetime of the binary runtime.

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.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h
File remoting/host/host_traits.h (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:18: class HostTraits final {
On 2016/11/03 02:29:35, Hzj_jie wrote:
> On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> > 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?
> 
> 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.

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.

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

That's fine. We could have static list for them:

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

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

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:35: void Erase(const std::string& key);
On 2016/11/03 02:29:35, Hzj_jie wrote:
> On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> > Do you really need this? Not sure we will ever want to erase traits.
> 
> 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?

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.

[Sergey Ulanov, 2016-11-03 19:05:48]

On 2016/11/03 03:20:32, Hzj_jie wrote:
> Looks like private reviews in http://codereview.chromium.org are discouraged.
Since
> this change will eventually be public, I removed protected flag of this
change.
> But all the documents are still for Google internal only.

I suggest either removing the links from the CL description or moving these docs
to chromium.org where they can be public.

[Hzj_jie, 2016-11-03 20:38:59]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2016-11-03 20:39:19]

Description was changed from

==========
[Chromoting] HostAttributes with tests

This change adds HostAttributes, which is a singleton object to represent all
attributes of the host, into host. Host uses this component to build a
comma-separated string, and send to client to indicate the capability of host
experiments. The design of HostAttributes can be found at
http://shortn/_J7i5IOexlO.

This is part of Chromoting Host Experiment feature, overall design doc is @
http://shortn/_7q3ylJYRD3.

BUG=650926
==========

to

==========
[Chromoting] HostAttributes with tests

This change adds HostAttributes, which is a singleton object to represent all
attributes of the host, into host. Host uses this component to build a
comma-separated string, and send to client to indicate the capability of host
experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926
==========

[commit-bot: I haz the power, 2016-11-03 20:39:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-03 21:33:31]

Moving the document to chromium.org is not doable. I removed the links from this
change.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.cc
File remoting/host/host_traits.cc (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.c...
remoting/host/host_traits.cc:20: static HostTraits* instance = new HostTraits();
On 2016/11/03 19:05:34, Sergey Ulanov wrote:
> On 2016/11/03 02:29:34, Hzj_jie wrote:
> > On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> > > 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.
> > 
> > I think this is a typical scenario of singleton. Only one instance of this
> class
> > is needed during the lifetime of the binary runtime.
> 
> 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.

Done.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h
File remoting/host/host_traits.h (right):

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:18: class HostTraits final {
On 2016/11/03 19:05:34, Sergey Ulanov wrote:
> On 2016/11/03 02:29:35, Hzj_jie wrote:
> > On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> > > 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?
> > 
> > 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.
> 
> 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.
> 
> > 
> > Also, not all the attributes are dynamic evaluated, say OS family, they
won't
> > change during the lifetime of the binary.
> 
> That's fine. We could have static list for them:
> 
> struct HostStaticAttributes {
>   char* name;
>   char* value;
> };
> 
> constexpr HostStaticAttributes g_static_attributes[] = {
>  { "os", OS },
> };

Acknowledged.

https://codereview.chromium.org/2464293002/diff/1/remoting/host/host_traits.h...
remoting/host/host_traits.h:35: void Erase(const std::string& key);
On 2016/11/03 19:05:34, Sergey Ulanov wrote:
> On 2016/11/03 02:29:35, Hzj_jie wrote:
> > On 2016/11/02 20:10:32, Sergey Ulanov wrote:
> > > Do you really need this? Not sure we will ever want to erase traits.
> > 
> > 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?
> 
> 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.

Done.

[Sergey Ulanov, 2016-11-03 22:15:34]

I'm still not sure why we need dynamic type to store the attributes instead of a
static list.

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
File remoting/host/directx_attributes.cc (right):

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
remoting/host/directx_attributes.cc:14:
host_attributes().Register("DirectX-Capturer",
What is host_attributes() and where is it defined? I don't see it in this
patchset.

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/host_attr...
File remoting/host/host_attributes.h (right):

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/host_attr...
remoting/host/host_attributes.h:44: std::map<std::string,
base::Callback<bool()>> dynamic_values_;
I still think we don't need set/map here instead of vector<>. It's possible to
verify presence of duplicate entries using std::find()

[commit-bot: I haz the power, 2016-11-03 22:16:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 22:16:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2016-11-03 22:17:42]

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
File remoting/host/directx_attributes.cc (right):

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
remoting/host/directx_attributes.cc:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
Do we really need one file per attribute, instead of a single file listing all
attributes for each platform?

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
remoting/host/directx_attributes.cc:7: #if defined(OS_WIN)
I think you need to include build/build_config.h to get OS_WIN

[Hzj_jie, 2016-11-04 00:17:49]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 00:19:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-04 00:20:33]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 00:21:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-04 00:21:24]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-11-04 01:00:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 01:00:24]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Hzj_jie, 2016-11-04 01:58:58]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 02:00:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-04 02:02:33]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-11-04 02:56:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 02:56:06]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Hzj_jie, 2016-11-04 19:18:09]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2016-11-04 19:18:20]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-11-04 19:18:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-04 20:18:46]

Description was changed from

==========
[Chromoting] HostAttributes with tests

This change adds HostAttributes, which is a singleton object to represent all
attributes of the host, into host. Host uses this component to build a
comma-separated string, and send to client to indicate the capability of host
experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926
==========

to

==========
[Chromoting] GetHostAttributes with tests

This change adds GetHostAttributes function, which returns a comma-separated
string to represent all attributes of the host, into host. Host sends the result
of this function to client to indicate the capability of host experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926
==========

[commit-bot: I haz the power, 2016-11-04 21:49:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 21:49:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Hzj_jie, 2016-11-05 00:32:19]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 00:32:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-05 00:32:47]

Patchset #4 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-11-05 02:51:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-05 02:51:30]

Dry run: This issue passed the CQ dry run.

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

I have added some comments to explain several decisions.

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
File remoting/host/directx_attributes.cc (right):

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
remoting/host/directx_attributes.cc:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2016/11/03 22:17:42, Sergey Ulanov wrote:
> Do we really need one file per attribute, instead of a single file listing all
> attributes for each platform?

Done.

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
remoting/host/directx_attributes.cc:7: #if defined(OS_WIN)
On 2016/11/03 22:17:42, Sergey Ulanov wrote:
> I think you need to include build/build_config.h to get OS_WIN

Done.

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/directx_a...
remoting/host/directx_attributes.cc:14:
host_attributes().Register("DirectX-Capturer",
On 2016/11/03 22:15:34, Sergey Ulanov wrote:
> What is host_attributes() and where is it defined? I don't see it in this
> patchset.

I forgot to change this file.

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/host_attr...
File remoting/host/host_attributes.h (right):

https://codereview.chromium.org/2464293002/diff/40001/remoting/host/host_attr...
remoting/host/host_attributes.h:44: std::map<std::string,
base::Callback<bool()>> dynamic_values_;
On 2016/11/03 22:15:34, Sergey Ulanov wrote:
> I still think we don't need set/map here instead of vector<>. It's possible to
> verify presence of duplicate entries using std::find()

I have moved this logic into the test case.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:22: static StaticAttribute
Create(DynamicAttribute&& attribute) {
MSVC does not allow to change a constexpr object, it simply crashes. So this
function is not constexpr.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:31: DynamicAttribute attribute;
I do not think it's a good idea to evaluate the StaticAttribute in static object
direct, it will impact startup time if the evaluation is costly. So a
StaticAttribute uses a DynamicAttribute, and caches its result in value field.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes_win.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:27: static StaticAttribute
kStaticAttributes[] = {
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.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:28: StaticAttribute::Create( {
"Debug-Build", &IsDebug } ),
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]

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes.cc (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.cc:57: #define SIZE_OF_ARRAY(x, y)
static_cast<int>(sizeof(x) / sizeof(y))
Add something in kDynamicAttributes and use arraysize() from base/macros.h

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.cc:73: i < SIZE_OF_ARRAY(kDynamicAttributes,
StaticAttribute);
type is wrong here. This macro could be implemented as (sizeof(x) /
sizeof(x[0])), i.e. you don't really need the second argument, but it's better
to use arraysize().

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:12: struct DynamicAttribute {
I don't think we need this to be in the header.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:14: bool(* value)();
this is not a good name for a function. maybe get_value_func?

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:31: DynamicAttribute attribute;
On 2016/11/05 05:29:28, Hzj_jie wrote:
> I do not think it's a good idea to evaluate the StaticAttribute in static
object
> direct, it will impact startup time if the evaluation is costly.

We are not allowed to have any static initializers at all. But I don't
understand why static values have to be evaluated dynamically at all. They
should have static values set during compile time, e.g.

struct StaticAttribute {
  const char* name;
  bool value;
};

StaticAttribute kStaticAttributes = {
  {"IsDebug", 
#if defined(NDEBUG)
   false
#else
   true
#endif
  },
  ...
}

Or for simplicity all attributes could be "dynamic" if they have to be evaluated
in runtime.

> So a
> StaticAttribute uses a DynamicAttribute, and caches its result in value field.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes_win.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:15: inline constexpr bool IsDebug() {
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.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:28: StaticAttribute::Create( {
"Debug-Build", &IsDebug } ),
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:10:23]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 01:11:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes.cc (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.cc:57: #define SIZE_OF_ARRAY(x, y)
static_cast<int>(sizeof(x) / sizeof(y))
On 2016/11/07 20:11:33, Sergey Ulanov wrote:
> Add something in kDynamicAttributes and use arraysize() from base/macros.h

Done.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.cc:73: i < SIZE_OF_ARRAY(kDynamicAttributes,
StaticAttribute);
On 2016/11/07 20:11:33, Sergey Ulanov wrote:
> type is wrong here. This macro could be implemented as (sizeof(x) /
> sizeof(x[0])), i.e. you don't really need the second argument, but it's better
> to use arraysize().

Done.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:12: struct DynamicAttribute {
On 2016/11/07 20:11:33, Sergey Ulanov wrote:
> I don't think we need this to be in the header.

Done.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:14: bool(* value)();
On 2016/11/07 20:11:33, Sergey Ulanov wrote:
> this is not a good name for a function. maybe get_value_func?

Done.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes.h:31: DynamicAttribute attribute;
On 2016/11/07 20:11:33, Sergey Ulanov wrote:
> On 2016/11/05 05:29:28, Hzj_jie wrote:
> > I do not think it's a good idea to evaluate the StaticAttribute in static
> object
> > direct, it will impact startup time if the evaluation is costly.
> 
> We are not allowed to have any static initializers at all. But I don't
> understand why static values have to be evaluated dynamically at all. They
> should have static values set during compile time, e.g.
> 
> struct StaticAttribute {
>   const char* name;
>   bool value;
> };
> 
> StaticAttribute kStaticAttributes = {
>   {"IsDebug", 
> #if defined(NDEBUG)
>    false
> #else
>    true
> #endif
>   },
>   ...
> }
> 
> Or for simplicity all attributes could be "dynamic" if they have to be
evaluated
> in runtime.
> 
> > So a
> > StaticAttribute uses a DynamicAttribute, and caches its result in value
field.
> 

See my comments in host_attributes_win.h

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes_win.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:15: inline constexpr bool IsDebug() {
On 2016/11/07 20:11:33, Sergey Ulanov wrote:
> 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.

Done.

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:28: StaticAttribute::Create( {
"Debug-Build", &IsDebug } ),
On 2016/11/07 20:11:33, Sergey Ulanov wrote:
> 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.

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}.

[commit-bot: I haz the power, 2016-11-08 01:53:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 01:53:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Hzj_jie, 2016-11-08 02:12:54]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2016-11-08 02:13:32]

Patchset #5 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-11-08 02:13:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-08 02:13:46]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2016-11-08 02:13:48]

Patchset #5 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-11-08 02:14:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 02:33:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 02:33:42]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hzj_jie, 2016-11-08 02:35:49]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2016-11-08 02:35:54]

Patchset #5 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-11-08 02:36:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 03:32:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 03:32:44]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes_win.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:28: StaticAttribute::Create( {
"Debug-Build", &IsDebug } ),
On 2016/11/08 01:29:54, Hzj_jie wrote:
> 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}.

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?

https://codereview.chromium.org/2464293002/diff/220001/remoting/host/host_att...
File remoting/host/host_attributes.cc (right):

https://codereview.chromium.org/2464293002/diff/220001/remoting/host/host_att...
remoting/host/host_attributes.cc:13: #if defined(OS_WIN)
nit: add empty line here

https://codereview.chromium.org/2464293002/diff/220001/remoting/host/host_att...
remoting/host/host_attributes.cc:97: value = attribute.get_value_func();
There is a data race because value is not atomic. This problem is easy to avoid
by making all attributes dynamic.

[Hzj_jie, 2016-11-09 01:58:21]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 01:58:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
File remoting/host/host_attributes_win.h (right):

https://codereview.chromium.org/2464293002/diff/140001/remoting/host/host_att...
remoting/host/host_attributes_win.h:28: StaticAttribute::Create( {
"Debug-Build", &IsDebug } ),
On 2016/11/08 20:11:05, Sergey Ulanov wrote:
> On 2016/11/08 01:29:54, Hzj_jie wrote:
> > 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}.
> 
> 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?

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

https://codereview.chromium.org/2464293002/diff/220001/remoting/host/host_att...
File remoting/host/host_attributes.cc (right):

https://codereview.chromium.org/2464293002/diff/220001/remoting/host/host_att...
remoting/host/host_attributes.cc:13: #if defined(OS_WIN)
On 2016/11/08 20:11:05, Sergey Ulanov wrote:
> nit: add empty line here

Done.

https://codereview.chromium.org/2464293002/diff/220001/remoting/host/host_att...
remoting/host/host_attributes.cc:97: value = attribute.get_value_func();
On 2016/11/08 20:11:05, Sergey Ulanov wrote:
> There is a data race because value is not atomic. This problem is easy to
avoid
> by making all attributes dynamic.

Done.

[commit-bot: I haz the power, 2016-11-09 03:42:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 03:42:36]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-11-10 23:03:23]

LGTM

[Hzj_jie, 2016-11-10 23:35:36]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-11-10 23:36:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 23:52:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 23:52:14]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Hzj_jie, 2016-11-10 23:54:15]

On 2016/11/10 23:52:14, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

Android build has been broken,
../../content/browser/renderer_host/render_widget_host_view_android.cc:1197:42:
error: no 'void
content::RenderWidgetHostViewAndroid::SetSynchronousCompositorClient(content::SynchronousCompositorClient*)'
member function declared in class 'content::RenderWidgetHostViewAndroid'
       SynchronousCompositorClient* client) {

I am waiting for the fix.

[Hzj_jie, 2016-11-11 01:05:13]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-11-11 01:05:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 01:51:48]

Description was changed from

==========
[Chromoting] GetHostAttributes with tests

This change adds GetHostAttributes function, which returns a comma-separated
string to represent all attributes of the host, into host. Host sends the result
of this function to client to indicate the capability of host experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926
==========

to

==========
[Chromoting] GetHostAttributes with tests

This change adds GetHostAttributes function, which returns a comma-separated
string to represent all attributes of the host, into host. Host sends the result
of this function to client to indicate the capability of host experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926
==========

[commit-bot: I haz the power, 2016-11-11 01:51:49]

Committed patchset #6 (id:240001)

[commit-bot: I haz the power, 2016-11-11 02:00:54]

Description was changed from

==========
[Chromoting] GetHostAttributes with tests

This change adds GetHostAttributes function, which returns a comma-separated
string to represent all attributes of the host, into host. Host sends the result
of this function to client to indicate the capability of host experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926
==========

to

==========
[Chromoting] GetHostAttributes with tests

This change adds GetHostAttributes function, which returns a comma-separated
string to represent all attributes of the host, into host. Host sends the result
of this function to client to indicate the capability of host experiments.

This is part of Chromoting Host Experiment feature.

BUG=650926

Committed: https://crrev.com/3010c8d9f1cb51a31f1dd22c42fa68b7eeacf27a
Cr-Commit-Position: refs/heads/master@{#431451}
==========

[commit-bot: I haz the power, 2016-11-11 02:00:59]

Patchset 6 (id:??) landed as
https://crrev.com/3010c8d9f1cb51a31f1dd22c42fa68b7eeacf27a
Cr-Commit-Position: refs/heads/master@{#431451}