Generate a static struct from fieldtrial_testing_config

Generate a static struct from fieldtrial_testing_config

This is gives Chromium developers visibility into what features are active, but hidden behind field trial testing while developing Chromium.

There is already a JSON file in Chromium with which field trials should be active. The goal is make a code generator to output a struct based off the contents of this JSON file. The next step after this is to create a mechanism to activate the trials defined by this generated struct.

BUG=504075
Committed: https://crrev.com/970b79a8adc8c3fa3593054681b6d4bc5ae3ec94
Cr-Commit-Position: refs/heads/master@{#338416}

[danduong, 2015-06-24 19:49:00]

danduong@chromium.org changed reviewers:
+ asvitkine@chromium.org, nednguyen@google.com

[danduong, 2015-06-24 23:36:19]

ned: I've updated the fieldtrial_util to only take 1 json instead of 2 since
that was the way people have seemed to used it anyways. It's likely that we'll
eventually remove this entirely since I plan to build this json file into chrome
browser build.

[danduong, 2015-06-24 23:39:02]

danduong@chromium.org changed reviewers:
+ beaudoin@chromium.org

[danduong, 2015-06-24 23:40:56]

On 2015/06/24 at 23:36:19, danduong wrote:
> ned: I've updated the fieldtrial_util to only take 1 json instead of 2 since
that was the way people have seemed to used it anyways. It's likely that we'll
eventually remove this entirely since I plan to build this json file into chrome
browser build.

Philippe, PTAL @ my modifications to the build configs and change,
json_to_struct, and my implmentation of fieldtrial_to_struct.

[nednguyen, 2015-06-25 00:10:36]

On 2015/06/24 23:40:56, danduong wrote:
> On 2015/06/24 at 23:36:19, danduong wrote:
> > ned: I've updated the fieldtrial_util to only take 1 json instead of 2 since
> that was the way people have seemed to used it anyways. It's likely that we'll
> eventually remove this entirely since I plan to build this json file into
chrome
> browser build.
> 
> Philippe, PTAL @ my modifications to the build configs and change,
> json_to_struct, and my implmentation of fieldtrial_to_struct.

Does this change work with the ref build?

[danduong, 2015-06-25 16:59:09]

On 2015/06/25 at 00:10:36, nednguyen wrote:
> On 2015/06/24 23:40:56, danduong wrote:
> > On 2015/06/24 at 23:36:19, danduong wrote:
> > > ned: I've updated the fieldtrial_util to only take 1 json instead of 2
since
> > that was the way people have seemed to used it anyways. It's likely that
we'll
> > eventually remove this entirely since I plan to build this json file into
chrome
> > browser build.
> > 
> > Philippe, PTAL @ my modifications to the build configs and change,
> > json_to_struct, and my implmentation of fieldtrial_to_struct.
> 
> Does this change work with the ref build?

Should work.

[nednguyen, 2015-06-25 17:00:47]

tools/perf/ owner lgtm

[beaudoin, 2015-07-06 15:24:13]

On 2015/06/25 17:00:47, nednguyen wrote:
> tools/perf/ owner lgtm

Modifs to config and json_to_struct LGTM.

Glad to see this little tool being used somewhere else! :)

[Alexei Svitkine (slow), 2015-07-08 16:54:54]

https://codereview.chromium.org/1209743002/diff/80001/tools/json_to_struct/js...
File tools/json_to_struct/json_to_struct.py (right):

https://codereview.chromium.org/1209743002/diff/80001/tools/json_to_struct/js...
tools/json_to_struct/json_to_struct.py:177: description_filename,
schema_filename):
Nit: Align

https://codereview.chromium.org/1209743002/diff/80001/tools/json_to_struct/js...
tools/json_to_struct/json_to_struct.py:178: head = HEAD % (datetime.now().year,
schema_filename, description_filename)
Add a comment about what this function does and document the params.

https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
File tools/variations/fieldtrial_to_struct.py (right):

https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
tools/variations/fieldtrial_to_struct.py:87: json_to_struct.GenerateStruct(
Can you add some tests for this implementation? I'd like to see mock inputs +
tests for expected C++ output.

[Alexei Svitkine (slow), 2015-07-08 17:07:20]

Also please expand the CL description to provide more context. What you have in
the bug description is good, so you can base it off that.

[danduong, 2015-07-08 23:33:46]

On 2015/07/08 at 17:07:20, asvitkine wrote:
> Also please expand the CL description to provide more context. What you have
in the bug description is good, so you can base it off that.

Done.

[danduong, 2015-07-09 00:12:54]

https://codereview.chromium.org/1209743002/diff/80001/tools/json_to_struct/js...
File tools/json_to_struct/json_to_struct.py (right):

https://codereview.chromium.org/1209743002/diff/80001/tools/json_to_struct/js...
tools/json_to_struct/json_to_struct.py:177: description_filename,
schema_filename):
On 2015/07/08 at 16:54:54, Alexei Svitkine wrote:
> Nit: Align

Done.

https://codereview.chromium.org/1209743002/diff/80001/tools/json_to_struct/js...
tools/json_to_struct/json_to_struct.py:178: head = HEAD % (datetime.now().year,
schema_filename, description_filename)
On 2015/07/08 at 16:54:53, Alexei Svitkine wrote:
> Add a comment about what this function does and document the params.

Done.

https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
File tools/variations/fieldtrial_to_struct.py (right):

https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
tools/variations/fieldtrial_to_struct.py:87: json_to_struct.GenerateStruct(
On 2015/07/08 at 16:54:54, Alexei Svitkine wrote:
> Can you add some tests for this implementation? I'd like to see mock inputs +
tests for expected C++ output.

I added a unittest for the most interesting part of the code (conversion of
fieldtrial_testing_config into a description format). My thought is that most of
the machinery is already covered by the other unit tests.

[Alexei Svitkine (slow), 2015-07-09 16:55:33]

https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
File tools/variations/fieldtrial_to_struct.py (right):

https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
tools/variations/fieldtrial_to_struct.py:87: json_to_struct.GenerateStruct(
On 2015/07/09 00:12:54, danduong wrote:
> On 2015/07/08 at 16:54:54, Alexei Svitkine wrote:
> > Can you add some tests for this implementation? I'd like to see mock inputs
+
> tests for expected C++ output.
> 
> I added a unittest for the most interesting part of the code (conversion of
> fieldtrial_testing_config into a description format). My thought is that most
of
> the machinery is already covered by the other unit tests.

I think it's worth having a test with an expectation for the resulting C++ code.
This way, we know exactly what's being generated and won't be bitten by changes
to the underlying machinery that may break us silently.

[danduong, 2015-07-09 19:38:39]

On 2015/07/09 at 16:55:33, asvitkine wrote:
>
https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
> File tools/variations/fieldtrial_to_struct.py (right):
> 
>
https://codereview.chromium.org/1209743002/diff/80001/tools/variations/fieldt...
> tools/variations/fieldtrial_to_struct.py:87: json_to_struct.GenerateStruct(
> On 2015/07/09 00:12:54, danduong wrote:
> > On 2015/07/08 at 16:54:54, Alexei Svitkine wrote:
> > > Can you add some tests for this implementation? I'd like to see mock
inputs +
> > tests for expected C++ output.
> > 
> > I added a unittest for the most interesting part of the code (conversion of
> > fieldtrial_testing_config into a description format). My thought is that
most of
> > the machinery is already covered by the other unit tests.
> 
> I think it's worth having a test with an expectation for the resulting C++
code. This way, we know exactly what's being generated and won't be bitten by
changes to the underlying machinery that may break us silently.

OK. Added another test for this.

[Alexei Svitkine (slow), 2015-07-09 19:48:28]

LGTM % remaining comment. Thanks!

https://codereview.chromium.org/1209743002/diff/120001/tools/variations/unitt...
File tools/variations/unittest_data/expected_output.h (right):

https://codereview.chromium.org/1209743002/diff/120001/tools/variations/unitt...
tools/variations/unittest_data/expected_output.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
This test will fail in 2016, since the year seems to be dynamically generated by
the script.

Please fix it so that this doesn't happen, either by:
  - Omitting this header comment in the expected files and checking Contains()
instead of Equals().
  - Or substituting the year string with a placeholder.

Or something else along those lines.

[danduong, 2015-07-09 21:55:37]

On 2015/07/09 at 19:48:28, asvitkine wrote:
> LGTM % remaining comment. Thanks!
> 
>
https://codereview.chromium.org/1209743002/diff/120001/tools/variations/unitt...
> File tools/variations/unittest_data/expected_output.h (right):
> 
>
https://codereview.chromium.org/1209743002/diff/120001/tools/variations/unitt...
> tools/variations/unittest_data/expected_output.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
> This test will fail in 2016, since the year seems to be dynamically generated
by the script.
> 
> Please fix it so that this doesn't happen, either by:
>   - Omitting this header comment in the expected files and checking Contains()
instead of Equals().
>   - Or substituting the year string with a placeholder.
> 
> Or something else along those lines.

Good catch!

[danduong, 2015-07-09 21:57:49]

danduong@chromium.org changed reviewers:
+ brettw@chromium.org

[danduong, 2015-07-09 21:58:33]

brettw: for gn and gyp review

[brettw, 2015-07-10 19:49:04]

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
File chrome/common/variations/BUILD.gn (right):

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:7: component("fieldtrial_testing_config") {
Delete this, actions are separate targets in GN. It's a bug in GN that it allows
you to nest targets.

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:37: out_dir = get_path_info("variations",
"gen_dir")
This get_path_info will return
"//out/foo/gen/chrome/common/variations/variations" which isn't what you want
(you're resolving the file "variations" against the current directory).

What you want is just the builtin variable target_gen_dir. You can delete your
out_dir variable altogether.

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:54: visibility = [ "*" ]
Delete this.

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:55: sources =
get_target_outputs(":fieldtrial_testing_config_action")
Before here, start a new target:
  source_set("fieldtrial_testing_config") {
component is unnecessary (you don't want this to be a shared library).

[danduong, 2015-07-10 22:41:37]

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
File chrome/common/variations/BUILD.gn (right):

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:37: out_dir = get_path_info("variations",
"gen_dir")
On 2015/07/10 at 19:49:04, brettw wrote:
> This get_path_info will return
"//out/foo/gen/chrome/common/variations/variations" which isn't what you want
(you're resolving the file "variations" against the current directory).
> 
> What you want is just the builtin variable target_gen_dir. You can delete your
out_dir variable altogether.

Done.

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:54: visibility = [ "*" ]
On 2015/07/10 at 19:49:04, brettw wrote:
> Delete this.

Done.

https://codereview.chromium.org/1209743002/diff/160001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:55: sources =
get_target_outputs(":fieldtrial_testing_config_action")
On 2015/07/10 at 19:49:04, brettw wrote:
> Before here, start a new target:
>   source_set("fieldtrial_testing_config") {
> component is unnecessary (you don't want this to be a shared library).

Done.

[brettw, 2015-07-10 23:05:33]

GN lgtm

https://codereview.chromium.org/1209743002/diff/200001/chrome/common/variatio...
File chrome/common/variations/BUILD.gn (right):

https://codereview.chromium.org/1209743002/diff/200001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:5: import("//build/config/ui.gni")
I don't think this is used for anything, you should be able to delete it.

[danduong, 2015-07-10 23:11:24]

https://codereview.chromium.org/1209743002/diff/200001/chrome/common/variatio...
File chrome/common/variations/BUILD.gn (right):

https://codereview.chromium.org/1209743002/diff/200001/chrome/common/variatio...
chrome/common/variations/BUILD.gn:5: import("//build/config/ui.gni")
On 2015/07/10 at 23:05:33, brettw wrote:
> I don't think this is used for anything, you should be able to delete it.

Done.

[danduong, 2015-07-10 23:11:37]

The CQ bit was checked by danduong@chromium.org

[danduong, 2015-07-10 23:11:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
beaudoin@chromium.org, nednguyen@google.com, asvitkine@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1209743002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-07-10 23:11:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209743002/220001

[commit-bot: I haz the power, 2015-07-10 23:22:53]

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

[commit-bot: I haz the power, 2015-07-10 23:22:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[danduong, 2015-07-10 23:49:24]

The CQ bit was checked by danduong@chromium.org

[danduong, 2015-07-10 23:49:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
beaudoin@chromium.org, nednguyen@google.com, brettw@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1209743002/#ps240001
(title: "add +x file permission")

[commit-bot: I haz the power, 2015-07-10 23:50:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209743002/240001

[commit-bot: I haz the power, 2015-07-11 00:07:14]

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

[commit-bot: I haz the power, 2015-07-11 00:07:15]

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

[danduong, 2015-07-11 00:16:40]

The CQ bit was checked by danduong@chromium.org

[commit-bot: I haz the power, 2015-07-11 00:17:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209743002/240001

[commit-bot: I haz the power, 2015-07-11 01:04:29]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-07-11 01:05:40]

Patchset 13 (id:??) landed as
https://crrev.com/970b79a8adc8c3fa3593054681b6d4bc5ae3ec94
Cr-Commit-Position: refs/heads/master@{#338416}