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

0 views
Skip to first unread message

Rafał Godlewski (Gerrit)

unread,
6:06 AM (9 hours ago) 6:06 AM
to Chirag Arora, Chromium LUCI CQ, Sylvain Defresne, Arthur Milchior, Elly, Chromium Metrics Reviews, Luc Nguyen, 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 Arthur Milchior, Chirag Arora, Elly, Josh Simmons, Luc Nguyen, Sylvain Defresne and Theresa Wellington

Rafał Godlewski voted and added 1 comment

Votes added by Rafał Godlewski

Code-Review+1

1 comment

Patchset-level comments
File-level comment, Patchset 6 (Latest):
Rafał Godlewski . resolved

icb/webauthn and icb/credential_exchange lgtm

Open in Gerrit

Related details

Attention is currently required from:
  • Arthur Milchior
  • Chirag Arora
  • Elly
  • Josh Simmons
  • Luc Nguyen
  • 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: 6
Gerrit-Owner: Chirag Arora <heyc...@google.com>
Gerrit-Reviewer: Arthur Milchior <arthurm...@chromium.org>
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: Arthur Milchior <arthurm...@chromium.org>
Gerrit-Attention: Josh Simmons <simmo...@google.com>
Gerrit-Attention: Elly <elly...@chromium.org>
Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>
Gerrit-Attention: Chirag Arora <heyc...@google.com>
Gerrit-Comment-Date: Thu, 23 Apr 2026 10:05:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Arthur Milchior (Gerrit)

unread,
6:09 AM (9 hours ago) 6:09 AM
to Chirag Arora, Rafał Godlewski, Chromium LUCI CQ, Sylvain Defresne, Elly, Chromium Metrics Reviews, Luc Nguyen, 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, Luc Nguyen, Sylvain Defresne and Theresa Wellington

Arthur Milchior added 1 comment

Patchset-level comments
Arthur Milchior . unresolved

Please don’t put 7 reviewers. There not a single file I own that Sylvain, for example, don’t own, and it’s not like you need my input specifically.
I suspect it’s the same for other people.

Add me back if you really need my input, and if so explains why please

Open in Gerrit

Related details

Attention is currently required from:
Gerrit-Attention: Josh Simmons <simmo...@google.com>
Gerrit-Attention: Elly <elly...@chromium.org>
Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>
Gerrit-Attention: Chirag Arora <heyc...@google.com>
Gerrit-Comment-Date: Thu, 23 Apr 2026 10:09:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Chirag Arora (Gerrit)

unread,
7:09 AM (8 hours ago) 7:09 AM
to Rafał Godlewski, Chromium LUCI CQ, Sylvain Defresne, Elly, Chromium Metrics Reviews, Luc Nguyen, 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 1 comment

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

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: 9
    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 11:09:39 +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

    Rafał Godlewski (Gerrit)

    unread,
    7:48 AM (7 hours ago) 7:48 AM
    to Chirag Arora, Chromium LUCI CQ, Sylvain Defresne, Elly, Chromium Metrics Reviews, Luc Nguyen, 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, Luc Nguyen, Sylvain Defresne and Theresa Wellington

    Rafał Godlewski voted Code-Review+1

    Code-Review+1
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Chirag Arora
    • Elly
    • Josh Simmons
    • Luc Nguyen
    • 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
      Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>
      Gerrit-Attention: Chirag Arora <heyc...@google.com>
      Gerrit-Comment-Date: Thu, 23 Apr 2026 11:48:38 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Luc Nguyen (Gerrit)

      unread,
      11:11 AM (4 hours ago) 11:11 AM
      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, Sylvain Defresne and Theresa Wellington

      Luc Nguyen voted and added 3 comments

      Votes added by Luc Nguyen

      Code-Review+1

      3 comments

      Patchset-level comments
      File-level comment, Patchset 9 (Latest):
      Luc Nguyen . resolved

      nice thanks!

      File components/metrics/metrics_reporting_choice_service.cc
      Line 68, Patchset 9 (Latest):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

      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?

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Chirag Arora
      • 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: 9
        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-Attention: Chirag Arora <heyc...@google.com>
        Gerrit-Comment-Date: Thu, 23 Apr 2026 15:11:14 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Elly (Gerrit)

        unread,
        12:11 PM (3 hours ago) 12:11 PM
        to Chirag Arora, 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 Chirag Arora, Josh Simmons, Sylvain Defresne and Theresa Wellington

        Elly added 1 comment

        Commit Message
        Line 7, Patchset 9 (Latest):Refactor kMetricsRportingEnabled checks for all platforms
        Elly . unresolved

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

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Chirag Arora
        Gerrit-Attention: Sylvain Defresne <sdef...@chromium.org>
        Gerrit-Attention: Chirag Arora <heyc...@google.com>
        Gerrit-Comment-Date: Thu, 23 Apr 2026 16:11:21 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy
        Reply all
        Reply to author
        Forward
        0 new messages