Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804633003:
Add base::FeatureParam<> struct (Closed)
|
Created:
3 years, 8 months ago by sfiera Modified:
3 years, 3 months ago CC:
chromium-reviews, danakj+watch_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd base::FeatureParam<> struct
Bundles information for a struct into a single place:
* Parameter name
* Associated feature
* Value type (string, bool, int, double, or enum)
* Default value
* For enum-valued parameters, the set of possible enum values and the
associated strings
On its own, this makes it easy to avoid certain kinds of errors (e.g.
using a param with the wrong feature, type, or default value) because
everything is declared together.
It would also make it possible to infer more easily from the source
whether a server-side configuration is valid.
Review-Url: https://chromiumcodereview.appspot.com/2804633003
Cr-Commit-Position: refs/heads/master@{#503177}
Committed: https://chromium.googlesource.com/chromium/src/+/01757be6de494235b94aad25b0d9b58e79382655
Patch Set 1 #Patch Set 2 : Add const where windows requires it #Patch Set 3 : Remove windows-incompatible constexpr #
Total comments: 10
Patch Set 4 : review #Patch Set 5 : Better nocompile test #Patch Set 6 : rebase #
Total comments: 12
Patch Set 7 : Tests #Patch Set 8 : remove ntp_snippets part #Patch Set 9 : Fix doc #Patch Set 10 : Add enum class test #Patch Set 11 : rebase #Patch Set 12 : Remove constexpr for Windows #
Messages
Total messages: 47 (34 generated)
The CQ bit was checked by sfiera@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...
sfiera@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei, I've left a bunch of TODOs in field_trial_params.h that are mostly points to clarify in the review. I made some design decisions in the API that you might differ on, and you're the OWNER :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sfiera@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: 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_...)
The CQ bit was checked by sfiera@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: 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...)
Sorry for the review delay. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... File base/metrics/field_trial_params.cc (right): https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.cc:130: int FeatureParam<int>::Get() const { What happens if someone tries to use a type that we don't have a Get() defined for - e.g. uint32_t? Will it be a compile error? (If so, that's fine, but we should document in the .h comment about which types are supported). https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... File base/metrics/field_trial_params.h (right): https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:96: // TODO(sfiera): is this the way we want to parameterize for different types? I I think this is a good use of templates - since the type gets clearly defined at the declaration - which is nice documentation for the param. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:211: // provide? I think it's not to have one given we'll be encouraging the use of these new constructs instead. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:234: Enum Get() const { Can this be implemented out of line? If so, LogInvalidEnumValue() can be in the anon namespace in the .cc file. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:236: if (value.empty()) { Nit: No {}'s
I've added nocompile tests; you can check the error messages for misuse of the API there. I still haven't added unittests. I'm OOO next week and will pick this up again after I'm back. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... File base/metrics/field_trial_params.cc (right): https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.cc:130: int FeatureParam<int>::Get() const { On 2017/04/24 18:35:47, Alexei Svitkine (slow) wrote: > What happens if someone tries to use a type that we don't have a Get() defined > for - e.g. uint32_t? Will it be a compile error? (If so, that's fine, but we > should document in the .h comment about which types are supported). Yes, a compile error. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... File base/metrics/field_trial_params.h (right): https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:96: // TODO(sfiera): is this the way we want to parameterize for different types? I On 2017/04/24 18:35:47, Alexei Svitkine (slow) wrote: > I think this is a good use of templates - since the type gets clearly defined at > the declaration - which is nice documentation for the param. Acknowledged. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:211: // provide? On 2017/04/24 18:35:48, Alexei Svitkine (slow) wrote: > I think it's not to have one given we'll be encouraging the use of these new > constructs instead. Acknowledged. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:234: Enum Get() const { On 2017/04/24 18:35:48, Alexei Svitkine (slow) wrote: > Can this be implemented out of line? If so, LogInvalidEnumValue() can be in the > anon namespace in the .cc file. No, unfortunately. It needs to be instantiated for each enum type. https://codereview.chromium.org/2804633003/diff/40001/base/metrics/field_tria... base/metrics/field_trial_params.h:236: if (value.empty()) { On 2017/04/24 18:35:48, Alexei Svitkine (slow) wrote: > Nit: No {}'s Done.
The CQ bit was checked by sfiera@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: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
I'm sorry I let this review stagnate for so long - I've been super busy and now am disappearing for a week again (on vacation). I still think this is a good API to add and we should pursue this. So hoping to look at this again when I'm back.
Just rebased this, but I haven't done any more work on it otherwise.
Base code looks good % comment. I didn't finish reviewing all the downstream usage changes. Maybe worth splitting those into separate CLs just so that they can be better scrutinized during review? Since I found at least one mistake. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... File base/metrics/field_trial_params.h (right): https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:160: // If the feature is not set, or set to an invalid double value, then Get() will Nit: "invalid int value" https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:176: template <> Nit: Add comment for this too. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:211: constexpr Option(Enum value, const char* name) : value(value), name(name) {} Nit: Add empty line below it. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:212: Enum value; const? https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:216: template <int option_count> Nit: size_t? If you change it, also change the i in the for loop below. https://codereview.chromium.org/2804633003/diff/100001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2804633003/diff/100001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:38: &kBookmarkSuggestionsFeature, "bookmarks_consider_desktop_visits", false}; Seems you're changing logic as the previous code had true set.
I've removed the ntp_snippets part—that was useful while writing it to make sure that the API was good to use, but should be reviewed separately. The full test suite (regular and nocompile) is in place now. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... File base/metrics/field_trial_params.h (right): https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:160: // If the feature is not set, or set to an invalid double value, then Get() will On 2017/08/24 18:13:05, Alexei Svitkine (slow) wrote: > Nit: "invalid int value" Done. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:176: template <> On 2017/08/24 18:13:05, Alexei Svitkine (slow) wrote: > Nit: Add comment for this too. Done. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:211: constexpr Option(Enum value, const char* name) : value(value), name(name) {} On 2017/08/24 18:13:05, Alexei Svitkine (slow) wrote: > Nit: Add empty line below it. Done. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:212: Enum value; On 2017/08/24 18:13:05, Alexei Svitkine (slow) wrote: > const? Done. https://codereview.chromium.org/2804633003/diff/100001/base/metrics/field_tri... base/metrics/field_trial_params.h:216: template <int option_count> On 2017/08/24 18:13:05, Alexei Svitkine (slow) wrote: > Nit: size_t? If you change it, also change the i in the for loop below. Done. https://codereview.chromium.org/2804633003/diff/100001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2804633003/diff/100001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:38: &kBookmarkSuggestionsFeature, "bookmarks_consider_desktop_visits", false}; On 2017/08/24 18:13:05, Alexei Svitkine (slow) wrote: > Seems you're changing logic as the previous code had true set. Fixed.
The CQ bit was checked by sfiera@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: 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...)
Sorry for the delay again. LGTM!
The CQ bit was checked by sfiera@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sfiera@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...
sfiera@chromium.org changed reviewers: + gab@chromium.org
+gab for base/BUILD.gn This CL adds an additional no-compile test for base::FeatureParam<>.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/09/20 08:49:17, sfiera wrote: > +gab for base/BUILD.gn > > This CL adds an additional no-compile test for base::FeatureParam<>. LGTM, love NC tests :)
Thanks! Farewell, Rietveld…
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2804633003/#ps220001 (title: "Remove constexpr for Windows")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1505925120609520,
"parent_rev": "4d6535e2f2fa3fbb1e36b9a31f7e83685d9e38da", "commit_rev":
"01757be6de494235b94aad25b0d9b58e79382655"}
Message was sent while issue was closed.
Description was changed from
==========
Add base::FeatureParam<> struct
Bundles information for a struct into a single place:
* Parameter name
* Associated feature
* Value type (string, bool, int, double, or enum)
* Default value
* For enum-valued parameters, the set of possible enum values and the
associated strings
On its own, this makes it easy to avoid certain kinds of errors (e.g.
using a param with the wrong feature, type, or default value) because
everything is declared together.
It would also make it possible to infer more easily from the source
whether a server-side configuration is valid.
==========
to
==========
Add base::FeatureParam<> struct
Bundles information for a struct into a single place:
* Parameter name
* Associated feature
* Value type (string, bool, int, double, or enum)
* Default value
* For enum-valued parameters, the set of possible enum values and the
associated strings
On its own, this makes it easy to avoid certain kinds of errors (e.g.
using a param with the wrong feature, type, or default value) because
everything is declared together.
It would also make it possible to infer more easily from the source
whether a server-side configuration is valid.
Review-Url: https://chromiumcodereview.appspot.com/2804633003
Cr-Commit-Position: refs/heads/master@{#503177}
Committed:
https://chromium.googlesource.com/chromium/src/+/01757be6de494235b94aad25b0d9...
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/01757be6de494235b94aad25b0d9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
