Refactor kMetricsReportingEnabled checks for all platforms [chromium/src : main]

0 views
Skip to first unread message

Chirag Arora (Gerrit)

unread,
12:57 PM (2 hours ago) 12:57 PM
to Luc Nguyen, Rafał Godlewski, Chromium LUCI CQ, Sylvain Defresne, Elly, Chromium Metrics Reviews, Theresa Wellington, Josh Simmons, Matt Dembski, chromium...@chromium.org, derinel+wat...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, net-r...@chromium.org, webauthn...@chromium.org, asvitki...@chromium.org, chromiumme...@microsoft.com, filesapp...@chromium.org
Attention needed from Elly, Josh Simmons, Sylvain Defresne and Theresa Wellington

Chirag Arora added 1 comment

Commit Message
Line 7, Patchset 9:Refactor kMetricsRportingEnabled checks for all platforms
Elly . resolved

```suggestion
Refactor kMetricsReportingEnabled checks for all platforms
```

Chirag Arora

Fix applied.

Open in Gerrit

Related details

Attention is currently required from:
  • Elly
  • Josh Simmons
  • Sylvain Defresne
  • Theresa Wellington
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If00120a6abc6ee73171fb23978fd66409f3bbae2
Gerrit-Change-Number: 7786352
Gerrit-PatchSet: 10
Gerrit-Owner: Chirag Arora <heyc...@google.com>
Gerrit-Reviewer: Chirag Arora <heyc...@google.com>
Gerrit-Reviewer: Elly <elly...@chromium.org>
Gerrit-Reviewer: Josh Simmons <simmo...@google.com>
Gerrit-Reviewer: Luc Nguyen <lucn...@google.com>
Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>
Gerrit-Reviewer: Theresa Wellington <twell...@chromium.org>
Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
Gerrit-CC: Matt Dembski <dem...@google.com>
Gerrit-CC: gwsq
Gerrit-Attention: Theresa Wellington <twell...@chromium.org>
Gerrit-Attention: Josh Simmons <simmo...@google.com>
Gerrit-Attention: Elly <elly...@chromium.org>
Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>
Gerrit-Comment-Date: Thu, 23 Apr 2026 16:57:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Elly <elly...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Chirag Arora (Gerrit)

unread,
1:18 PM (2 hours ago) 1:18 PM
to Luc Nguyen, Rafał Godlewski, Chromium LUCI CQ, Sylvain Defresne, Elly, Chromium Metrics Reviews, Theresa Wellington, Josh Simmons, Matt Dembski, chromium...@chromium.org, derinel+wat...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, net-r...@chromium.org, webauthn...@chromium.org, asvitki...@chromium.org, chromiumme...@microsoft.com, filesapp...@chromium.org
Attention needed from Elly, Josh Simmons, Luc Nguyen, Rafał Godlewski, Sylvain Defresne and Theresa Wellington

Chirag Arora added 2 comments

File components/metrics/metrics_reporting_choice_service.cc
Line 68, Patchset 9:bool MetricsReportingChoiceService::IsMetricsReportingDisabled(
const PrefService* local_state) {
CHECK(local_state);
if (ShouldUseMetricsConsentRestructure(local_state)) {
return local_state->IsManagedPreference(prefs::kMetricsReportingLevel) &&
!IsBasicMetricsReportingEnabled(local_state);
}
return local_state->IsManagedPreference(prefs::kMetricsReportingEnabled) &&
!IsBasicMetricsReportingEnabled(local_state);
}
Luc Nguyen . unresolved

nit: this seems a bit confusing, there's both an `IsReportingEnabled()` function and an `IsReportingDisabled()` function, yet they may both return false at the same time? it's not super obvious to me which one should be used at which time, is the plan to consolidate these? maybe add a todo comment

Chirag Arora

Makes sense. Renamed to `IsMetricsReportingDisabledByPolicy`.

File components/metrics/metrics_service_accessor.cc
Line 28, Patchset 4 (Parent): if ((!base::FeatureList::GetInstance() ||
base::FeatureList::IsEnabled(
features::kRestructureMetricsConsentSettings)) &&
local_state->GetBoolean(prefs::kMetricsReportingMigrationDone)) {
// If FeatureList is not initialized yet (e.g., during early startup), we
// can't check the feature flag. But if the user has already migrated, we
// know they were in the experiment, so we should assume the feature is
// enabled to avoid downgrading the entropy provider or crash reporting
// unnecessarily.
switch (static_cast<MetricsReportingLevel>(
local_state->GetInteger(prefs::kMetricsReportingLevel))) {
case MetricsReportingLevel::kBasic:
case MetricsReportingLevel::kAdvanced:
return true;
default:
return false;
}
}

return local_state->GetBoolean(prefs::kMetricsReportingEnabled);
Luc Nguyen . unresolved

nit: i think this should be done in the parent CL that adds `IsBasicMetricsReportingEnabled()`?

Chirag Arora

It's being done to keep CLs small. Another CL will also be needed for replacing checks in the iOS codebase but it's not being added here since I'll create that from my local macOS copy of the repo. LMK what you think.

Chirag Arora

Added for iOS as well in this CL

Luc Nguyen

to be clear i didn't mean the whole CL, i meant this code specifically

in the parent CL you're moving/duplicating this exact code to a new `IsBasicMetricsReportingEnabled()` function ([link](https://chromium-review.git.corp.google.com/c/chromium/src/+/7784653/10/components/metrics/metrics_reporting_choice_service.cc)), seems pretty obvious to me that you should also remove this code in that other CL instead just completely duplicating it?

Chirag Arora

Makes sense. Moved to the CL below.

Open in Gerrit

Related details

Attention is currently required from:
  • Elly
  • Josh Simmons
  • Luc Nguyen
  • Rafał Godlewski
  • Sylvain Defresne
  • Theresa Wellington
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If00120a6abc6ee73171fb23978fd66409f3bbae2
    Gerrit-Change-Number: 7786352
    Gerrit-PatchSet: 11
    Gerrit-Owner: Chirag Arora <heyc...@google.com>
    Gerrit-Reviewer: Chirag Arora <heyc...@google.com>
    Gerrit-Reviewer: Elly <elly...@chromium.org>
    Gerrit-Reviewer: Josh Simmons <simmo...@google.com>
    Gerrit-Reviewer: Luc Nguyen <lucn...@google.com>
    Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
    Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>
    Gerrit-Reviewer: Theresa Wellington <twell...@chromium.org>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-CC: Matt Dembski <dem...@google.com>
    Gerrit-CC: gwsq
    Gerrit-Attention: Luc Nguyen <lucn...@google.com>
    Gerrit-Attention: Theresa Wellington <twell...@chromium.org>
    Gerrit-Attention: Josh Simmons <simmo...@google.com>
    Gerrit-Attention: Elly <elly...@chromium.org>
    Gerrit-Attention: Rafał Godlewski <rg...@google.com>
    Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>
    Gerrit-Comment-Date: Thu, 23 Apr 2026 17:18:31 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Luc Nguyen <lucn...@google.com>
    Comment-In-Reply-To: Chirag Arora <heyc...@google.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Luc Nguyen (Gerrit)

    unread,
    1:24 PM (2 hours ago) 1:24 PM
    to Chirag Arora, Rafał Godlewski, Chromium LUCI CQ, Sylvain Defresne, Elly, Chromium Metrics Reviews, Theresa Wellington, Josh Simmons, Matt Dembski, chromium...@chromium.org, derinel+wat...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, net-r...@chromium.org, webauthn...@chromium.org, asvitki...@chromium.org, chromiumme...@microsoft.com, filesapp...@chromium.org
    Attention needed from Chirag Arora, Elly, Josh Simmons, Rafał Godlewski, Sylvain Defresne and Theresa Wellington

    Luc Nguyen voted Code-Review+1

    Code-Review+1
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Chirag Arora
    • Elly
    • Josh Simmons
    • Rafał Godlewski
    • Sylvain Defresne
    • Theresa Wellington
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement is not satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: If00120a6abc6ee73171fb23978fd66409f3bbae2
        Gerrit-Change-Number: 7786352
        Gerrit-PatchSet: 11
        Gerrit-Owner: Chirag Arora <heyc...@google.com>
        Gerrit-Reviewer: Chirag Arora <heyc...@google.com>
        Gerrit-Reviewer: Elly <elly...@chromium.org>
        Gerrit-Reviewer: Josh Simmons <simmo...@google.com>
        Gerrit-Reviewer: Luc Nguyen <lucn...@google.com>
        Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
        Gerrit-Reviewer: Sylvain Defresne <sdef...@chromium.org>
        Gerrit-Reviewer: Theresa Wellington <twell...@chromium.org>
        Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
        Gerrit-CC: Matt Dembski <dem...@google.com>
        Gerrit-CC: gwsq
        Gerrit-Attention: Theresa Wellington <twell...@chromium.org>
        Gerrit-Attention: Josh Simmons <simmo...@google.com>
        Gerrit-Attention: Elly <elly...@chromium.org>
        Gerrit-Attention: Rafał Godlewski <rg...@google.com>
        Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>
        Gerrit-Attention: Chirag Arora <heyc...@google.com>
        Gerrit-Comment-Date: Thu, 23 Apr 2026 17:24:19 +0000
        Gerrit-HasComments: No
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Chirag Arora (Gerrit)

        unread,
        1:26 PM (2 hours ago) 1:26 PM
        to Luc Nguyen, Rafał Godlewski, Chromium LUCI CQ, Sylvain Defresne, Elly, Chromium Metrics Reviews, Theresa Wellington, Josh Simmons, Matt Dembski, chromium...@chromium.org, derinel+wat...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, net-r...@chromium.org, webauthn...@chromium.org, asvitki...@chromium.org, chromiumme...@microsoft.com, filesapp...@chromium.org
        Attention needed from Elly, Josh Simmons, Rafał Godlewski, Sylvain Defresne and Theresa Wellington

        Chirag Arora added 1 comment

        File components/metrics/metrics_reporting_choice_service.cc
        Line 68, Patchset 9:bool MetricsReportingChoiceService::IsMetricsReportingDisabled(
        const PrefService* local_state) {
        CHECK(local_state);
        if (ShouldUseMetricsConsentRestructure(local_state)) {
        return local_state->IsManagedPreference(prefs::kMetricsReportingLevel) &&
        !IsBasicMetricsReportingEnabled(local_state);
        }
        return local_state->IsManagedPreference(prefs::kMetricsReportingEnabled) &&
        !IsBasicMetricsReportingEnabled(local_state);
        }
        Luc Nguyen . resolved

        nit: this seems a bit confusing, there's both an `IsReportingEnabled()` function and an `IsReportingDisabled()` function, yet they may both return false at the same time? it's not super obvious to me which one should be used at which time, is the plan to consolidate these? maybe add a todo comment

        Chirag Arora

        Makes sense. Renamed to `IsMetricsReportingDisabledByPolicy`.

        Chirag Arora

        Done

        Open in Gerrit

        Related details

        Attention is currently required from:
        Gerrit-Comment-Date: Thu, 23 Apr 2026 17:25:48 +0000
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy
        Reply all
        Reply to author
        Forward
        0 new messages