Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2296493002:
Merge all Field Trial Testing Configuration Together (Closed)
DescriptionMerge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h] \
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
Committed: https://crrev.com/74ce2fa9c04444812d985ef37ecd377e23e09145
Cr-Commit-Position: refs/heads/master@{#420112}
Patch Set 1 : Review #
Total comments: 13
Patch Set 2 : CR Feedback #Patch Set 3 : Rebase to eac7349 and add commenting support. #Patch Set 4 : Rebase to e3a7b31 #
Total comments: 5
Patch Set 5 : Add Comment #
Total comments: 2
Patch Set 6 : Rebase to 380781d #Patch Set 7 : Update Config (Found a Bug in Old->New Converter) #
Total comments: 2
Patch Set 8 : Switch to current_os #Patch Set 9 : Rebase to a67debf #Messages
Total messages: 57 (35 generated)
robliao@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: Please review this CL. Thanks!
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Enthusiastically seconded: Thanks for working on this! =D
https://codereview.chromium.org/2296493002/diff/20001/chrome/common/variation... File chrome/common/variations/BUILD.gn (right): https://codereview.chromium.org/2296493002/diff/20001/chrome/common/variation... chrome/common/variations/BUILD.gn:28: source = "//testing/variations/fieldtrial_testing_config.json" Nit: Just inline this on line 34 https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... File tools/variations/unittest_data/test_config.json (right): https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... tools/variations/unittest_data/test_config.json:10: "groups": [ We call these "experiments" server-side. How about keeping the same naming convention here?
https://codereview.chromium.org/2296493002/diff/20001/chrome/common/variation... File chrome/common/variations/BUILD.gn (right): https://codereview.chromium.org/2296493002/diff/20001/chrome/common/variation... chrome/common/variations/BUILD.gn:28: source = "//testing/variations/fieldtrial_testing_config.json" On 2016/08/30 17:59:15, Alexei Svitkine (slow) wrote: > Nit: Just inline this on line 34 This is also used in line 43. https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... File tools/variations/unittest_data/test_config.json (right): https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... tools/variations/unittest_data/test_config.json:10: "groups": [ On 2016/08/30 17:59:15, Alexei Svitkine (slow) wrote: > We call these "experiments" server-side. How about keeping the same naming > convention here? jwd suggested that the terminiology was trial and group. https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... If we called these experiments, we would call all of these studies (which is partially what I had in the previous CL that packaged all of the groups in C++). What's the preferred terminology for field trials in Chromium?
Sorry for the slow turnaround time here. Still catching from my vacation today. I'll try to do better tomorrow. https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... File tools/variations/unittest_data/test_config.json (right): https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... tools/variations/unittest_data/test_config.json:10: "groups": [ On 2016/08/30 18:16:43, robliao wrote: > On 2016/08/30 17:59:15, Alexei Svitkine (slow) wrote: > > We call these "experiments" server-side. How about keeping the same naming > > convention here? > > jwd suggested that the terminiology was trial and group. > https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... > > If we called these experiments, we would call all of these studies (which is > partially what I had in the previous CL that packaged all of the groups in C++). > > What's the preferred terminology for field trials in Chromium? It's a tricky question and I can see arguments for both sides. In this case, given this is making the name more prominent to users, I think making it more consistent with server-side terminology makes more sense. It's true that FieldTrial API uses "group" terminology, but it's also true that almost no one actually uses APIs that use that terminology - i.e. base::Feature or variation params is preferred these days and even when raw field trials are used, the API is usually FindFullName() which doesn't mention group. So my suggestion is to go with "study"/"experiments" just like server-side configs. I would also rename "group_name" field to just "name" now that "groups" is being spelled out explicitly. This way, the config actually becomes a subset of the server-side config, essentially. https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... tools/variations/unittest_data/test_config.json:30: "platforms": ["win"] Can you list platforms first? This is the convention for server-side configs and I think will be more natural to our users. (Both here but more importantly in the actual JSON file.)
Looks good % comments below and the naming that I mentioned in my previous reply. Let me know when you've addressed these and I can take another look. Thanks! https://codereview.chromium.org/2296493002/diff/20001/testing/variations/PRES... File testing/variations/PRESUBMIT.py (right): https://codereview.chromium.org/2296493002/diff/20001/testing/variations/PRES... testing/variations/PRESUBMIT.py:66: 'groups in Trial[%s]' % (file_path, trial))] Nit: There's a bunch of repetition in these messages. Can you make a helper function? The "Malformed config file %s: " + msg + " in Trial[%s]" is common to all these. https://codereview.chromium.org/2296493002/diff/20001/tools/variations/fieldt... File tools/variations/fieldtrial_to_struct.py (right): https://codereview.chromium.org/2296493002/diff/20001/tools/variations/fieldt... tools/variations/fieldtrial_to_struct.py:52: def _CreateTrial(trial_name, group_configs, platform): Please add a comment to this function now that it's less trivial.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/2296493002/diff/20001/testing/variations/PRES... File testing/variations/PRESUBMIT.py (right): https://codereview.chromium.org/2296493002/diff/20001/testing/variations/PRES... testing/variations/PRESUBMIT.py:66: 'groups in Trial[%s]' % (file_path, trial))] On 2016/08/31 20:17:27, Alexei Svitkine wrote: > Nit: There's a bunch of repetition in these messages. Can you make a helper > function? > > The "Malformed config file %s: " + msg + " in Trial[%s]" is common to all these. Done. https://codereview.chromium.org/2296493002/diff/20001/tools/variations/fieldt... File tools/variations/fieldtrial_to_struct.py (right): https://codereview.chromium.org/2296493002/diff/20001/tools/variations/fieldt... tools/variations/fieldtrial_to_struct.py:52: def _CreateTrial(trial_name, group_configs, platform): On 2016/08/31 20:17:27, Alexei Svitkine wrote: > Please add a comment to this function now that it's less trivial. Done. https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... File tools/variations/unittest_data/test_config.json (right): https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... tools/variations/unittest_data/test_config.json:10: "groups": [ On 2016/08/30 21:39:50, Alexei Svitkine wrote: > On 2016/08/30 18:16:43, robliao wrote: > > On 2016/08/30 17:59:15, Alexei Svitkine (slow) wrote: > > > We call these "experiments" server-side. How about keeping the same naming > > > convention here? > > > > jwd suggested that the terminiology was trial and group. > > > https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... > > > > If we called these experiments, we would call all of these studies (which is > > partially what I had in the previous CL that packaged all of the groups in > C++). > > > > What's the preferred terminology for field trials in Chromium? > > It's a tricky question and I can see arguments for both sides. > > In this case, given this is making the name more prominent to users, I think > making it more consistent with server-side terminology makes more sense. > > It's true that FieldTrial API uses "group" terminology, but it's also true that > almost no one actually uses APIs that use that terminology - i.e. base::Feature > or variation params is preferred these days and even when raw field trials are > used, the API is usually FindFullName() which doesn't mention group. > > So my suggestion is to go with "study"/"experiments" just like server-side > configs. I would also rename "group_name" field to just "name" now that "groups" > is being spelled out explicitly. This way, the config actually becomes a subset > of the server-side config, essentially. I made the json format changes. I'd like to adjust the C++ side after this change to isolate this change to python only. https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... tools/variations/unittest_data/test_config.json:30: "platforms": ["win"] On 2016/08/30 21:39:50, Alexei Svitkine wrote: > Can you list platforms first? This is the convention for server-side configs and > I think will be more natural to our users. (Both here but more importantly in > the actual JSON file.) Done. This also required rewriting the pretty printer.
Looks like comment support got added with https://chromium.googlesource.com/chromium/src/+/45e66c790f6b93dec90df68aaaf8... Updated the change to support this.
https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... File tools/variations/unittest_data/test_config.json (right): https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... tools/variations/unittest_data/test_config.json:10: "groups": [ On 2016/09/01 21:49:07, robliao wrote: > On 2016/08/30 21:39:50, Alexei Svitkine wrote: > > On 2016/08/30 18:16:43, robliao wrote: > > > On 2016/08/30 17:59:15, Alexei Svitkine (slow) wrote: > > > > We call these "experiments" server-side. How about keeping the same naming > > > > convention here? > > > > > > jwd suggested that the terminiology was trial and group. > > > > > > https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... > > > > > > If we called these experiments, we would call all of these studies (which is > > > partially what I had in the previous CL that packaged all of the groups in > > C++). > > > > > > What's the preferred terminology for field trials in Chromium? > > > > It's a tricky question and I can see arguments for both sides. > > > > In this case, given this is making the name more prominent to users, I think > > making it more consistent with server-side terminology makes more sense. > > > > It's true that FieldTrial API uses "group" terminology, but it's also true > that > > almost no one actually uses APIs that use that terminology - i.e. > base::Feature > > or variation params is preferred these days and even when raw field trials are > > used, the API is usually FindFullName() which doesn't mention group. > > > > So my suggestion is to go with "study"/"experiments" just like server-side > > configs. I would also rename "group_name" field to just "name" now that > "groups" > > is being spelled out explicitly. This way, the config actually becomes a > subset > > of the server-side config, essentially. > > I made the json format changes. I'd like to adjust the C++ side after this > change to isolate this change to python only. https://codereview.chromium.org/2319293006/ contains the C++ changes.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/09 00:48:02, robliao wrote: > https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... > File tools/variations/unittest_data/test_config.json (right): > > https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... > tools/variations/unittest_data/test_config.json:10: "groups": [ > On 2016/09/01 21:49:07, robliao wrote: > > On 2016/08/30 21:39:50, Alexei Svitkine wrote: > > > On 2016/08/30 18:16:43, robliao wrote: > > > > On 2016/08/30 17:59:15, Alexei Svitkine (slow) wrote: > > > > > We call these "experiments" server-side. How about keeping the same > naming > > > > > convention here? > > > > > > > > jwd suggested that the terminiology was trial and group. > > > > > > > > > > https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... > > > > > > > > If we called these experiments, we would call all of these studies (which > is > > > > partially what I had in the previous CL that packaged all of the groups in > > > C++). > > > > > > > > What's the preferred terminology for field trials in Chromium? > > > > > > It's a tricky question and I can see arguments for both sides. > > > > > > In this case, given this is making the name more prominent to users, I think > > > making it more consistent with server-side terminology makes more sense. > > > > > > It's true that FieldTrial API uses "group" terminology, but it's also true > > that > > > almost no one actually uses APIs that use that terminology - i.e. > > base::Feature > > > or variation params is preferred these days and even when raw field trials > are > > > used, the API is usually FindFullName() which doesn't mention group. > > > > > > So my suggestion is to go with "study"/"experiments" just like server-side > > > configs. I would also rename "group_name" field to just "name" now that > > "groups" > > > is being spelled out explicitly. This way, the config actually becomes a > > subset > > > of the server-side config, essentially. > > > > I made the json format changes. I'd like to adjust the C++ side after this > > change to isolate this change to python only. > > https://codereview.chromium.org/2319293006/ > contains the C++ changes. Thanks. Which CL will land first? Should I review the other one first and this one will be rebased on top of it? Or vice versa? (You didn't add me as a reviewer on the other one.)
On 2016/09/09 18:56:00, Alexei Svitkine (very slow) wrote: > On 2016/09/09 00:48:02, robliao wrote: > > > https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... > > File tools/variations/unittest_data/test_config.json (right): > > > > > https://codereview.chromium.org/2296493002/diff/20001/tools/variations/unitte... > > tools/variations/unittest_data/test_config.json:10: "groups": [ > > On 2016/09/01 21:49:07, robliao wrote: > > > On 2016/08/30 21:39:50, Alexei Svitkine wrote: > > > > On 2016/08/30 18:16:43, robliao wrote: > > > > > On 2016/08/30 17:59:15, Alexei Svitkine (slow) wrote: > > > > > > We call these "experiments" server-side. How about keeping the same > > naming > > > > > > convention here? > > > > > > > > > > jwd suggested that the terminiology was trial and group. > > > > > > > > > > > > > > > https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... > > > > > > > > > > If we called these experiments, we would call all of these studies > (which > > is > > > > > partially what I had in the previous CL that packaged all of the groups > in > > > > C++). > > > > > > > > > > What's the preferred terminology for field trials in Chromium? > > > > > > > > It's a tricky question and I can see arguments for both sides. > > > > > > > > In this case, given this is making the name more prominent to users, I > think > > > > making it more consistent with server-side terminology makes more sense. > > > > > > > > It's true that FieldTrial API uses "group" terminology, but it's also true > > > that > > > > almost no one actually uses APIs that use that terminology - i.e. > > > base::Feature > > > > or variation params is preferred these days and even when raw field trials > > are > > > > used, the API is usually FindFullName() which doesn't mention group. > > > > > > > > So my suggestion is to go with "study"/"experiments" just like server-side > > > > configs. I would also rename "group_name" field to just "name" now that > > > "groups" > > > > is being spelled out explicitly. This way, the config actually becomes a > > > subset > > > > of the server-side config, essentially. > > > > > > I made the json format changes. I'd like to adjust the C++ side after this > > > change to isolate this change to python only. > > > > https://codereview.chromium.org/2319293006/ > > contains the C++ changes. > > Thanks. Which CL will land first? Should I review the other one first and this > one will be rebased on top of it? Or vice versa? (You didn't add me as a > reviewer on the other one.) This CL will land first. The other CL is going through checks before I send it out.
Description was changed from ========== Merge all Field Trial Testing Configuration Together This change brings together many of the separate platform specific Field Trial Testing configurations together, reducing duplication. BUG=637095 ========== to ========== Merge all Field Trial Testing Configuration Together This change brings together many of the separate platform specific Field Trial Testing configurations together, reducing duplication. BUG=637095 ==========
Rebase to e3a7b31
https://codereview.chromium.org/2296493002/diff/140001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2296493002/diff/140001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:2: "AllBookmarks": [ Hmm, one thing I just realized - sorry should have thought of it before - does the new system mean that there's no way to have a different group tested for a study on different platforms? (And that we had this flexibility before.) Is there any such conflicts for existing data? https://codereview.chromium.org/2296493002/diff/140001/tools/variations/field... File tools/variations/fieldtrial_to_struct.py (right): https://codereview.chromium.org/2296493002/diff/140001/tools/variations/field... tools/variations/fieldtrial_to_struct.py:71: if study['groups']: Can you add a comment above this that explains this check? (e.g. in what case will it not have groups).
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/2296493002/diff/140001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2296493002/diff/140001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:2: "AllBookmarks": [ On 2016/09/19 16:59:38, Alexei Svitkine (very slow) wrote: > Hmm, one thing I just realized - sorry should have thought of it before - does > the new system mean that there's no way to have a different group tested for a > study on different platforms? (And that we had this flexibility before.) > > Is there any such conflicts for existing data? Do you mean different groups per platform? That's expressed as [ { "platforms": ["plat1"] "experiments": [ {experiment1}, {experiment2} ] } { "platforms": ["plat2"] "experiments": [ {experiment3} ...} } ] plat1 = experiment1 plat2 = experiment3 The experiments are added in array order for all matching platforms. The existing data did not have such conflicts that I know of (the automerger verifies the experiment config before merging the platform in). https://codereview.chromium.org/2296493002/diff/140001/tools/variations/field... File tools/variations/fieldtrial_to_struct.py (right): https://codereview.chromium.org/2296493002/diff/140001/tools/variations/field... tools/variations/fieldtrial_to_struct.py:71: if study['groups']: On 2016/09/19 16:59:39, Alexei Svitkine (very slow) wrote: > Can you add a comment above this that explains this check? (e.g. in what case > will it not have groups). Done.
LGTM % comment Thanks and really sorry for the super slow review. https://codereview.chromium.org/2296493002/diff/140001/testing/variations/fie... File testing/variations/fieldtrial_testing_config.json (right): https://codereview.chromium.org/2296493002/diff/140001/testing/variations/fie... testing/variations/fieldtrial_testing_config.json:2: "AllBookmarks": [ On 2016/09/19 17:16:26, robliao wrote: > On 2016/09/19 16:59:38, Alexei Svitkine (very slow) wrote: > > Hmm, one thing I just realized - sorry should have thought of it before - does > > the new system mean that there's no way to have a different group tested for a > > study on different platforms? (And that we had this flexibility before.) > > > > Is there any such conflicts for existing data? > > Do you mean different groups per platform? > > That's expressed as > [ > { > "platforms": ["plat1"] > "experiments": [ {experiment1}, {experiment2} ] > } > { > "platforms": ["plat2"] > "experiments": [ {experiment3} ...} > } > ] > plat1 = experiment1 > plat2 = experiment3 > > The experiments are added in array order for all matching platforms. > > The existing data did not have such conflicts that I know of (the automerger > verifies the experiment config before merging the platform in). Ah, perfect - then it should be fine. :) Probably worth mentioning in the docs about this case once it lands. :) https://codereview.chromium.org/2296493002/diff/180001/testing/variations/PRE... File testing/variations/PRESUBMIT.py (right): https://codereview.chromium.org/2296493002/diff/180001/testing/variations/PRE... testing/variations/PRESUBMIT.py:145: 'Invalid param (%s: %s) for Experiment[%s] in Study[%s]', Nit: I would hoist "in Study[%s]" out of the custom message and make it be there by default in _CreateMalformedConfig(). It does mean that some messages will now have it at the end instead of at the middle like the one on line 150, but I think that should be fine.
https://codereview.chromium.org/2296493002/diff/180001/testing/variations/PRE... File testing/variations/PRESUBMIT.py (right): https://codereview.chromium.org/2296493002/diff/180001/testing/variations/PRE... testing/variations/PRESUBMIT.py:145: 'Invalid param (%s: %s) for Experiment[%s] in Study[%s]', On 2016/09/19 18:57:02, Alexei Svitkine (very slow) wrote: > Nit: I would hoist "in Study[%s]" out of the custom message and make it be there > by default in _CreateMalformedConfig(). It does mean that some messages will now > have it at the end instead of at the middle like the one on line 150, but I > think that should be fine. Making the file_path common made sense because it was a nice header that we could move out. |study|, as you noted, generalizes less nicely in L150 and in addition L116, where we don't have a valid study name yet. While all error messages will begin with "Malformed config file %s:", we may not want all error messages to end with "in Study[%s]". Keeping in this way is also nice in that the most important part of the message is available in one place.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robliao@chromium.org changed reviewers: + thakis@chromium.org
thakis@chromium.org: Please review changes in chrome/common/variations/BUILD.gn Thanks!
Description was changed from
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
==========
to
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
/chromium/src/testing/variations/fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h]
new/fieldtrial_testing_config_*.[cc|h]
No material differences
==========
Description was changed from
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
/chromium/src/testing/variations/fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h]
new/fieldtrial_testing_config_*.[cc|h]
No material differences
==========
to
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
/chromium/src/testing/variations/fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h]
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
/chromium/src/testing/variations/fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h]
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
to
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h]
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
Description was changed from
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=/chromium/src/chrome/common/variations/fieldtrial_testing_config_schema.json
\
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h]
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
to
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h] \
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2296493002/diff/220001/chrome/common/variatio... File chrome/common/variations/BUILD.gn (right): https://codereview.chromium.org/2296493002/diff/220001/chrome/common/variatio... chrome/common/variations/BUILD.gn:48: "--platform=" + platform, Just passing current_os here is probably just as good?
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2296493002/diff/220001/chrome/common/variatio... File chrome/common/variations/BUILD.gn (right): https://codereview.chromium.org/2296493002/diff/220001/chrome/common/variatio... chrome/common/variations/BUILD.gn:48: "--platform=" + platform, On 2016/09/20 20:47:47, Nico wrote: > Just passing current_os here is probably just as good? Looks like it should do. Done. Using current_os for now to maintain same behavior. I wonder if target_os might be more suitable, although it looks like it might be the same thing in the end.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2296493002/#ps260001 (title: "Rebase to a67debf")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h] \
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
to
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h] \
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Description was changed from
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h] \
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
==========
to
==========
Merge all Field Trial Testing Configuration Together
This change brings together many of the separate platform specific
Field Trial Testing configurations together, reducing duplication.
BUG=637095
TEST=Manually Compare Old and New Resulting C++ files
* - ["android", "chromeos", "ios", "linux", "mac", "win"]
fieldtrial_to_struct_master.py \
fieldtrial_testing_config_*.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--output=old/fieldtrial_testing_config_*
fieldtrial_to_struct.py \
fieldtrial_testing_config.json \
--namespace=chrome_variations \
--schema=fieldtrial_testing_config_schema.json \
--platform=* \
--output=new/fieldtrial_testing_config_*
diff old/fieldtrial_testing_config_*.[cc|h] \
new/fieldtrial_testing_config_*.[cc|h]
No material differences.
Committed: https://crrev.com/74ce2fa9c04444812d985ef37ecd377e23e09145
Cr-Commit-Position: refs/heads/master@{#420112}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/74ce2fa9c04444812d985ef37ecd377e23e09145 Cr-Commit-Position: refs/heads/master@{#420112} |
