Alexis Hétu (Gerrit)

unread,

Apr 21, 2026, 1:17:27 PM (2 days ago) Apr 21

to Sourav Uttam Sinha, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Sourav Uttam Sinha

Alexis Hétu added 2 comments

File components/webauthn/ios/passkey_tab_helper.mm

Line 39, Patchset 15 (Latest):

constexpr base::TimeDelta kPasskeyUpgradeRecencyThreshold = base::Minutes(5);

std::string GetRpId(const GURL& url) {
  std::string rp_id = net::registry_controlled_domains::GetDomainAndRegistry(

Alexis Hétu . unresolved

missing comments

Line 356, Patchset 15 (Latest):

  std::string domain_rp_id =
      net::registry_controlled_domains::GetDomainAndRegistry(
          rp_id, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
  if (domain_rp_id.empty()) {
    domain_rp_id = rp_id;
  }

Alexis Hétu . unresolved

This code is almost the same as `GetRpId`. I *think* this can be deduplicated this way (to be verified):
```
std::string GetDomainAndRegistryOrHost(std::string_view host) {
  std::string domain = net::registry_controlled_domains::GetDomainAndRegistry(
      host, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
  return domain.empty() ? std::string(host) : domain;
}
```

1st call site:
```
std::string domain_rp_id = GetDomainAndRegistryOrHost(params.RpId());
```

2nd call site:
```
`GetDomainAndRegistryOrHost(form->url.host()) == domain_rp_id`
```

Open in Gerrit

Related details

Attention is currently required from:

Sourav Uttam Sinha

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sourav Uttam Sinha (Gerrit)

unread,

Apr 21, 2026, 1:41:18 PM (2 days ago) Apr 21

to Alexis Hétu, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Alexis Hétu

Sourav Uttam Sinha added 2 comments

File components/webauthn/ios/passkey_tab_helper.mm

Line 39, Patchset 15:constexpr base::TimeDelta kPasskeyUpgradeRecencyThreshold = base::Minutes(5);



std::string GetRpId(const GURL& url) {
  std::string rp_id = net::registry_controlled_domains::GetDomainAndRegistry(

Alexis Hétu . resolved

missing comments

Sourav Uttam Sinha

Done

Line 356, Patchset 15: std::string domain_rp_id =


      net::registry_controlled_domains::GetDomainAndRegistry(
          rp_id, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
  if (domain_rp_id.empty()) {
    domain_rp_id = rp_id;
  }

Alexis Hétu . resolved

This code is almost the same as `GetRpId`. I *think* this can be deduplicated this way (to be verified):
```
std::string GetDomainAndRegistryOrHost(std::string_view host) {
  std::string domain = net::registry_controlled_domains::GetDomainAndRegistry(
      host, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
  return domain.empty() ? std::string(host) : domain;
}
```

1st call site:
```
std::string domain_rp_id = GetDomainAndRegistryOrHost(params.RpId());
```

2nd call site:
```
`GetDomainAndRegistryOrHost(form->url.host()) == domain_rp_id`
```

Sourav Uttam Sinha

Done

Open in Gerrit

Related details

Attention is currently required from:

Alexis Hétu

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alexis Hétu (Gerrit)

unread,

Apr 22, 2026, 1:14:35 PM (yesterday) Apr 22

to Sourav Uttam Sinha, Tommy Martino, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Sourav Uttam Sinha and Tommy Martino

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

Code-Review

+1

1 comment

File components/webauthn/ios/passkey_tab_helper.mm

Line 356, Patchset 17:

  std::string rp_id = params.RpId();
  std::string domain_rp_id = GetDomainAndRegistryOrHost(rp_id);

Alexis Hétu . unresolved

Unnecessary string copy can be avoided:
```suggestion


  std::string domain_rp_id = GetDomainAndRegistryOrHost(params.RpId());
```

Open in Gerrit

Related details

Attention is currently required from:

Sourav Uttam Sinha
Tommy Martino

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sourav Uttam Sinha (Gerrit)

unread,

Apr 22, 2026, 1:30:53 PM (yesterday) Apr 22

to Alexis Hétu, Tommy Martino, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Tommy Martino

Sourav Uttam Sinha added 1 comment

File components/webauthn/ios/passkey_tab_helper.mm

Line 356, Patchset 17:

  std::string rp_id = params.RpId();
  std::string domain_rp_id = GetDomainAndRegistryOrHost(rp_id);

Alexis Hétu . resolved

Unnecessary string copy can be avoided:
```suggestion
  std::string domain_rp_id = GetDomainAndRegistryOrHost(params.RpId());
```

Sourav Uttam Sinha

Done

Open in Gerrit

Related details

Attention is currently required from:

Tommy Martino

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tommy Martino (Gerrit)

unread,

Apr 22, 2026, 3:00:00 PM (yesterday) Apr 22

to Sourav Uttam Sinha, Alexis Hétu, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Sourav Uttam Sinha

Tommy Martino added 1 comment

File components/webauthn/ios/passkey_tab_helper.h

Line 249, Patchset 19 (Latest): void OnGetPasswordStoreResults(

Tommy Martino . unresolved

This appears to be a deprecated method (see comment https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_store/password_store_consumer.h;l=60;drc=d1c430bf8d4e459a748e6ec0ba277eddd4fceb53)

Can you instead use the `OnGetPasswordStoreResultsOrErrorFrom` method?

As a bonus, that method receives a `std::vector<PasswordForm>` instead of a `std::vector<std::unique_ptr<PasswordForm>>`, which makes way more sense in this context.

Open in Gerrit

Related details

Attention is currently required from:

Sourav Uttam Sinha

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alexis Hétu (Gerrit)

unread,

Apr 22, 2026, 3:45:22 PM (yesterday) Apr 22

to Sourav Uttam Sinha, Tommy Martino, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Sourav Uttam Sinha

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

Code-Review

+1

1 comment

File components/webauthn/ios/passkey_tab_helper.h

Line 127, Patchset 19 (Latest): bool CanPerformAutomaticPasskeyUpgradeForTesting(

Alexis Hétu . unresolved

I just realized you don't need this function. PasskeyTabHelperTest is already a friend class to this class, so it can call CanPerformAutomaticPasskeyUpgrade directly.

satisfied_requirement

unsatisfied_requirement

open

diffy

Sourav Uttam Sinha (Gerrit)

unread,

12:34 PM (6 hours ago) 12:34 PM

to Alexis Hétu, Tommy Martino, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Tommy Martino

Sourav Uttam Sinha added 2 comments

File components/webauthn/ios/passkey_tab_helper.h

Line 249, Patchset 19: void OnGetPasswordStoreResults(

Tommy Martino . resolved

This appears to be a deprecated method (see comment https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_store/password_store_consumer.h;l=60;drc=d1c430bf8d4e459a748e6ec0ba277eddd4fceb53)
Can you instead use the `OnGetPasswordStoreResultsOrErrorFrom` method?
As a bonus, that method receives a `std::vector<PasswordForm>` instead of a `std::vector<std::unique_ptr<PasswordForm>>`, which makes way more sense in this context.

Sourav Uttam Sinha

Done

Line 127, Patchset 19: bool CanPerformAutomaticPasskeyUpgradeForTesting(

Alexis Hétu . resolved

I just realized you don't need this function. PasskeyTabHelperTest is already a friend class to this class, so it can call CanPerformAutomaticPasskeyUpgrade directly.

Sourav Uttam Sinha

Done

Open in Gerrit

Related details

Attention is currently required from:

Tommy Martino

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alexis Hétu (Gerrit)

unread,

1:43 PM (5 hours ago) 1:43 PM

to Sourav Uttam Sinha, Tommy Martino, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Sourav Uttam Sinha and Tommy Martino

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

Code-Review

+0

1 comment

File components/webauthn/ios/passkey_tab_helper.mm

Line 868, Patchset 21 (Latest):


  if (std::holds_alternative<password_manager::PasswordStoreBackendError>(
          results_or_error)) {
    std::vector<std::string> request_ids_to_process;
    for (const auto& [id, params] : registration_requests_) {
      if (params.Type() ==
          PasskeyRequestParams::RequestType::kConditionalCreate) {
        request_ids_to_process.push_back(id);
      }
    }
    for (const std::string& id : request_ids_to_process) {
      auto it = registration_requests_.find(id);
      if (it != registration_requests_.end()) {
        DeferToRenderer(it->second.RequestInfo(), it->second.Type());
        registration_requests_.erase(it);
      }
    }
    return;
  }

  const auto& results =
      std::get<password_manager::LoginsResult>(results_or_error);

  std::vector<std::string> request_ids_to_process;
  for (const auto& [id, params] : registration_requests_) {
    if (params.Type() ==
        PasskeyRequestParams::RequestType::kConditionalCreate) {
      request_ids_to_process.push_back(id);
    }
  }

  for (const std::string& id : request_ids_to_process) {
    auto it = registration_requests_.find(id);
    if (it == registration_requests_.end()) {
      continue;
    }

    const RegistrationRequestParams& params = it->second;

    if (CanPerformAutomaticPasskeyUpgrade(params, results)) {
      StartPasskeyCreation(id);
    } else {
      DeferToRenderer(params.RequestInfo(), params.Type());
      registration_requests_.erase(it);
    }
  }

Alexis Hétu . unresolved

There a lot of code duplication here. Here's a quick improvement:
```suggestion

  std::vector<std::string> request_ids_to_process;
  for (const auto& [id, params] : registration_requests_) {
    if (params.Type() == PasskeyRequestParams::RequestType::kConditionalCreate) {
      request_ids_to_process.push_back(id);
    }
  }

  const std::vector<password_manager::PasswordForm>* results = nullptr;
  if (std::holds_alternative<password_manager::LoginsResult>(results_or_error)) {
    results = &std::get<password_manager::LoginsResult>(results_or_error);
  }

  for (const std::string& id : request_ids_to_process) {
    auto it = registration_requests_.find(id);
    if (it == registration_requests_.end()) {
      continue;
    }

    const RegistrationRequestParams& params = it->second;

    // Start creation only if we successfully retrieved results AND the upgrade is allowed.
    if (results && CanPerformAutomaticPasskeyUpgrade(params, *results)) {
      StartPasskeyCreation(id);
    } else {
      DeferToRenderer(params.RequestInfo(), params.Type());
      registration_requests_.erase(it);
    }
  }
```

Open in Gerrit

Related details

Attention is currently required from:

Sourav Uttam Sinha
Tommy Martino

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sourav Uttam Sinha (Gerrit)

unread,

2:11 PM (4 hours ago) 2:11 PM

to Alexis Hétu, Tommy Martino, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, marq+...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, vasilii+watchlis...@chromium.org, tmartino+tran...@chromium.org, gcasto+w...@chromium.org, derinel+wat...@google.com, ios-r...@chromium.org, webauthn...@chromium.org

Attention needed from Alexis Hétu

Sourav Uttam Sinha added 1 comment

File components/webauthn/ios/passkey_tab_helper.mm

Line 868, Patchset 21:

Alexis Hétu . resolved

There a lot of code duplication here. Here's a quick improvement:
```suggestion

  std::vector<std::string> request_ids_to_process;
  for (const auto& [id, params] : registration_requests_) {
    if (params.Type() == PasskeyRequestParams::RequestType::kConditionalCreate) {
      request_ids_to_process.push_back(id);
    }
  }

  const std::vector<password_manager::PasswordForm>* results = nullptr;
  if (std::holds_alternative<password_manager::LoginsResult>(results_or_error)) {
    results = &std::get<password_manager::LoginsResult>(results_or_error);
  }

  for (const std::string& id : request_ids_to_process) {
    auto it = registration_requests_.find(id);
    if (it == registration_requests_.end()) {
      continue;
    }

    const RegistrationRequestParams& params = it->second;

    // Start creation only if we successfully retrieved results AND the upgrade is allowed.
    if (results && CanPerformAutomaticPasskeyUpgrade(params, *results)) {
      StartPasskeyCreation(id);
    } else {
      DeferToRenderer(params.RequestInfo(), params.Type());
      registration_requests_.erase(it);
    }
  }
```

Sourav Uttam Sinha

Yes, sorry about that. Thanks for noticing it.

Open in Gerrit

Related details

Attention is currently required from:

Alexis Hétu

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[iOSPasskeyM3] Add recency check for automatic passkey upgrades [chromium/src : main]

Alexis Hétu (Gerrit)

Alexis Hétu added 2 comments

Related details

Sourav Uttam Sinha (Gerrit)

Sourav Uttam Sinha added 2 comments

Related details

Alexis Hétu (Gerrit)

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

1 comment

Related details

Sourav Uttam Sinha (Gerrit)

Sourav Uttam Sinha added 1 comment

Related details

Tommy Martino (Gerrit)

Tommy Martino added 1 comment

Related details

Alexis Hétu (Gerrit)

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

1 comment

Sourav Uttam Sinha (Gerrit)

Sourav Uttam Sinha added 2 comments

Related details

Alexis Hétu (Gerrit)

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

1 comment

Related details

Sourav Uttam Sinha (Gerrit)

Sourav Uttam Sinha added 1 comment

Related details