Add UMA logging for base::FeatureList::IsEnabled() calls. [chromium/src : main]
Alexei Svitkine (Gerrit)
Alexei Svitkine added 7 comments![]( "Open in Gerrit")
constexpr uint32_t kCachedLogGeneralMask = 0x00010000;Add comments to these and also can they be defined somewhere close to the comment above mutable std::atomic<uint32_t> cached_value?
One option is to make a type alias for this type and having the comment above the type alias and have the constants following, together in the same place.
// We need to use a loop to preserve the logging bits (bits 16 and 17),Let's make a helper function to set the specified bits that you call here.
void FeatureList::RegisterFeatureAccess(const Feature& feature,Make this in anon namespace function at the top of the file, since it doesn't need to be in the .h
if ((expected & logging_mask) == logging_mask) {This is the same condition as the while, so not needed.
if (feature.cached_value.compare_exchange_weak(expected, new_value,Nit: Let's add a comment mentioning that this call will update `expected` if the value doesn't match.
BASE_FEATURE(kEarlyFeature, FEATURE_DISABLED_BY_DEFAULT);Put these at the top of the file in the anon namespace.
<enum name="HashFeatureName">Nit: FeatureNameHash
Related details
- Arkadiusz Tomczak
- Ramon Cano Aparicio
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arkadiusz Tomczak (Gerrit)
Arkadiusz Tomczak voted and added 7 comments![]( "Open in Gerrit")
Votes added by Arkadiusz Tomczak
| Commit-Queue | +1 |
7 comments
constexpr uint32_t kCachedLogGeneralMask = 0x00010000;Add comments to these and also can they be defined somewhere close to the comment above mutable std::atomic<uint32_t> cached_value?
One option is to make a type alias for this type and having the comment above the type alias and have the constants following, together in the same place.
I created type alias and moved the constants.
// We need to use a loop to preserve the logging bits (bits 16 and 17),Let's make a helper function to set the specified bits that you call here.
Separated to helper function.
void FeatureList::RegisterFeatureAccess(const Feature& feature,Make this in anon namespace function at the top of the file, since it doesn't need to be in the .h
Moved to anonymous namespace.
This is the same condition as the while, so not needed.
Right, removed the condition.
if (feature.cached_value.compare_exchange_weak(expected, new_value,Nit: Let's add a comment mentioning that this call will update `expected` if the value doesn't match.
Added a comment.
BASE_FEATURE(kEarlyFeature, FEATURE_DISABLED_BY_DEFAULT);Put these at the top of the file in the anon namespace.
Done
<enum name="HashFeatureName">Arkadiusz TomczakNit: FeatureNameHash
Renamed.
Related details
- Alexei Svitkine
- Ramon Cano Aparicio
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 2 comments![]( "Open in Gerrit")
public:Don't introduce a public section in the middle. Just put these at the top of the normal public section.
void RegisterFeatureAccess(Just pass the base::Feature to this instead of cached_value and feature_name separately.
Related details
- Arkadiusz Tomczak
- Ramon Cano Aparicio
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ramon Cano Aparicio (Gerrit)
Ramon Cano Aparicio voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ramon Cano Aparicio
| Code-Review | +1 |
2 comments
public:Don't introduce a public section in the middle. Just put these at the top of the normal public section.
Same for the private one, it's duplicated
Related details
- Arkadiusz Tomczak
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arkadiusz Tomczak (Gerrit)
Arkadiusz Tomczak added 2 comments![]( "Open in Gerrit")
Ramon Cano AparicioDon't introduce a public section in the middle. Just put these at the top of the normal public section.
Same for the private one, it's duplicated
I've restructured the definitions and accompanying comments.
void RegisterFeatureAccess(Just pass the base::Feature to this instead of cached_value and feature_name separately.
We cannot do that explicitly since the `cached_value` is a private field of Feature. I brought back the RegisterFeatureAccess as static method of FeatureList, since it's a friend class and this way it will have access to this field.
Related details
- Alexei Svitkine
- Ramon Cano Aparicio
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ramon Cano Aparicio (Gerrit)
Ramon Cano Aparicio voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ramon Cano Aparicio
| Code-Review | +1 |
LGTM % comments
// Registers the feature access to the appropriate histograms.
static void RegisterFeatureAccess(const Feature& feature,
Feature::FeatureStateCache logging_mask);Let's move this below go/cstyle#Declaration_Order
Also, make order the methods in .cc file with the same order as they are declared here.
Related details
- Alexei Svitkine
- Arkadiusz Tomczak
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arkadiusz Tomczak (Gerrit)
Arkadiusz Tomczak added 1 comment![]( "Open in Gerrit")
// Registers the feature access to the appropriate histograms.
static void RegisterFeatureAccess(const Feature& feature,
Feature::FeatureStateCache logging_mask);Let's move this below go/cstyle#Declaration_Order
Also, make order the methods in .cc file with the same order as they are declared here.
Moved it to a better suiting location. Methods in .cc don't follow the order from .h, but right new method is at the end of static methods block, right before first private method `FinalizeInitialization`. If we want to strictly follow this guidance, we would need to land a small refactor first.
Related details
- Alexei Svitkine
- Ramon Cano Aparicio
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ramon Cano Aparicio (Gerrit)
Ramon Cano Aparicio voted Code-Review+1![]( "Open in Gerrit")
- Alexei Svitkine
- Arkadiusz Tomczak
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |