[iOS][CX] Handle import transitions [chromium/src : main]

0 views
Skip to first unread message

Tommy Martino (Gerrit)

unread,
Nov 10, 2025, 5:28:45 PMNov 10
to Rafał Godlewski, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Rafał Godlewski

Tommy Martino added 5 comments

File ios/chrome/browser/credential_exchange/coordinator/credential_import_mediator.mm
Line 93, Patchset 7 (Latest): ImportDataItem* item =
[[ImportDataItem alloc] initWithType:ImportDataItemType::kPasswords
status:ImportDataItemImportStatus::kImported
count:results.number_imported];
[_consumer setImportDataItem:item];
Tommy Martino . unresolved

Nit: A bit weird that here we create a local var for `item` and just above we instantiate it directly inline, let's be consistent

File ios/chrome/browser/credential_exchange/model/credential_importer.h
Line 51, Patchset 7 (Latest):// credential type is handled in a separate async task. Creates a barrier
// closure to analyze the results when all tasks finish.
Tommy Martino . unresolved

Using a barrier closure is an implementation detail. Let's instead say that "results are analyzed once all tasks complete."

File ios/chrome/browser/credential_exchange/model/credential_importer.mm
Line 33, Patchset 7 (Latest):constexpr int kSupportedCredentialTypesAmount = 2;
Tommy Martino . unresolved

/s/Amount/Count

Line 107, Patchset 7 (Latest): base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
Tommy Martino . unresolved

Is it important that these run in sequence, or can we just use ThreadPool?

Line 109, Patchset 7 (Latest): [weakSelf startImportingPasswords:barrierClosure];
Tommy Martino . unresolved

I'm assuming neither of these two tasks performs any blocking operations (e.g. file I/O) but let me know if I'm wrong

Open in Gerrit

Related details

Attention is currently required from:
  • Rafał Godlewski
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement 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: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
Gerrit-Change-Number: 7132421
Gerrit-PatchSet: 7
Gerrit-Owner: Rafał Godlewski <rg...@google.com>
Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
Gerrit-Reviewer: Tommy Martino <tmar...@chromium.org>
Gerrit-Attention: Rafał Godlewski <rg...@google.com>
Gerrit-Comment-Date: Mon, 10 Nov 2025 22:28:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Rafał Godlewski (Gerrit)

unread,
Nov 11, 2025, 9:31:06 AMNov 11
to AyeAye, Tommy Martino, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Tommy Martino

Rafał Godlewski added 4 comments

File ios/chrome/browser/credential_exchange/coordinator/credential_import_mediator.mm
Line 93, Patchset 7: ImportDataItem* item =

[[ImportDataItem alloc] initWithType:ImportDataItemType::kPasswords
status:ImportDataItemImportStatus::kImported
count:results.number_imported];
[_consumer setImportDataItem:item];
Tommy Martino . resolved

Nit: A bit weird that here we create a local var for `item` and just above we instantiate it directly inline, let's be consistent

Rafał Godlewski

I was thinking ahead that this `item` will need to set its `invalidCount` property eventually, but there was no indication of that in the code..

File ios/chrome/browser/credential_exchange/model/credential_importer.h
Line 51, Patchset 7:// credential type is handled in a separate async task. Creates a barrier

// closure to analyze the results when all tasks finish.
Tommy Martino . resolved

Using a barrier closure is an implementation detail. Let's instead say that "results are analyzed once all tasks complete."

Rafał Godlewski

Done

File ios/chrome/browser/credential_exchange/model/credential_importer.mm
Line 33, Patchset 7:constexpr int kSupportedCredentialTypesAmount = 2;
Tommy Martino . resolved

/s/Amount/Count

Rafał Godlewski

Done

Line 109, Patchset 7: [weakSelf startImportingPasswords:barrierClosure];
Tommy Martino . resolved

I'm assuming neither of these two tasks performs any blocking operations (e.g. file I/O) but let me know if I'm wrong

Rafał Godlewski

Correct, they do not perform any blocking operations.

Open in Gerrit

Related details

Attention is currently required from:
  • Tommy Martino
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement 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: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
Gerrit-Change-Number: 7132421
Gerrit-PatchSet: 9
Gerrit-Owner: Rafał Godlewski <rg...@google.com>
Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
Gerrit-Reviewer: Tommy Martino <tmar...@chromium.org>
Gerrit-Attention: Tommy Martino <tmar...@chromium.org>
Gerrit-Comment-Date: Tue, 11 Nov 2025 14:30:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tommy Martino <tmar...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Rafał Godlewski (Gerrit)

unread,
Nov 12, 2025, 10:59:40 AMNov 12
to AyeAye, Tommy Martino, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Tommy Martino

Rafał Godlewski added 1 comment

File ios/chrome/browser/credential_exchange/model/credential_importer.mm
Line 107, Patchset 7: base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
Tommy Martino . resolved

Is it important that these run in sequence, or can we just use ThreadPool?

Rafał Godlewski

Summary of some offline discussions:

  • some parts of both importers logic needs to be run on the main thread (e.g. interaction with PasswordStore API and with the passkey sync bridge)
  • the sensible course of action would be to handle translation from NSObjects to C++ in a non-main thread and reply on main thread to trigger the respecitve //components importer logic, which then can choose to do stuff on whichever sequence it wants
  • some of the existing importer implementation could be refactored to (internally) offload more stuff on a non-main thread
  • the documentation around the importer class could be improved to specify which functions should be run on which sequence

Hence, with this CL I'll proceed with having a PostTaskAndReplyWithResult to translate NSObjects async and call the importer logic on the original sequence.

Open in Gerrit

Related details

Attention is currently required from:
  • Tommy Martino
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • 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: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
    Gerrit-Change-Number: 7132421
    Gerrit-PatchSet: 11
    Gerrit-Owner: Rafał Godlewski <rg...@google.com>
    Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
    Gerrit-Reviewer: Tommy Martino <tmar...@chromium.org>
    Gerrit-Attention: Tommy Martino <tmar...@chromium.org>
    Gerrit-Comment-Date: Wed, 12 Nov 2025 15:59:23 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Tommy Martino (Gerrit)

    unread,
    Nov 12, 2025, 4:00:12 PMNov 12
    to Rafał Godlewski, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
    Attention needed from Rafał Godlewski

    Tommy Martino voted and added 1 comment

    Votes added by Tommy Martino

    Code-Review+1

    1 comment

    File ios/chrome/browser/credential_exchange/model/credential_importer.mm
    Line 262, Patchset 11 (Latest): _allCredentialTypesProcessedClosure.Reset();
    Tommy Martino . unresolved

    I don't think this is actually necessary; BarrierCallback already invalidates itself when the last invocation runs.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Rafał Godlewski
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement 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: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
      Gerrit-Change-Number: 7132421
      Gerrit-PatchSet: 11
      Gerrit-Owner: Rafał Godlewski <rg...@google.com>
      Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
      Gerrit-Reviewer: Tommy Martino <tmar...@chromium.org>
      Gerrit-Attention: Rafał Godlewski <rg...@google.com>
      Gerrit-Comment-Date: Wed, 12 Nov 2025 21:00:03 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Rafał Godlewski (Gerrit)

      unread,
      Nov 13, 2025, 2:45:48 AMNov 13
      to Tommy Martino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

      Rafał Godlewski added 1 comment

      File ios/chrome/browser/credential_exchange/model/credential_importer.mm
      Line 262, Patchset 11: _allCredentialTypesProcessedClosure.Reset();
      Tommy Martino . resolved

      I don't think this is actually necessary; BarrierCallback already invalidates itself when the last invocation runs.

      Rafał Godlewski

      Done

      Open in Gerrit

      Related details

      Attention set is empty
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • 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: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
        Gerrit-Change-Number: 7132421
        Gerrit-PatchSet: 12
        Gerrit-Owner: Rafał Godlewski <rg...@google.com>
        Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
        Gerrit-Reviewer: Tommy Martino <tmar...@chromium.org>
        Gerrit-Comment-Date: Thu, 13 Nov 2025 07:45:32 +0000
        satisfied_requirement
        open
        diffy

        Rafał Godlewski (Gerrit)

        unread,
        Nov 13, 2025, 2:45:51 AMNov 13
        to Tommy Martino, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

        Rafał Godlewski voted Commit-Queue+2

        Commit-Queue+2
        Open in Gerrit

        Related details

        Attention set is empty
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • 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: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
        Gerrit-Change-Number: 7132421
        Gerrit-PatchSet: 12
        Gerrit-Owner: Rafał Godlewski <rg...@google.com>
        Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
        Gerrit-Reviewer: Tommy Martino <tmar...@chromium.org>
        Gerrit-Comment-Date: Thu, 13 Nov 2025 07:45:35 +0000
        Gerrit-HasComments: No
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        open
        diffy

        Chromium LUCI CQ (Gerrit)

        unread,
        Nov 13, 2025, 4:23:18 AMNov 13
        to Rafał Godlewski, Tommy Martino, AyeAye, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

        Chromium LUCI CQ submitted the change with unreviewed changes

        Unreviewed changes

        11 is the latest approved patch-set.
        The change was submitted with unreviewed changes in the following files:

        ```
        The name of the file: ios/chrome/browser/credential_exchange/model/credential_importer.mm
        Insertions: 0, Deletions: 2.

        The diff is too large to show. Please review the diff.
        ```

        Change information

        Commit message:
        [iOS][CX] Handle import transitions

        This CL adds a skeleton of the remaining logic in credential importer
        and adds a bunch of TODOs to annotate it. Notable changes:
        * processing each credential type is triggered in its separate async task
        * barrier closure is added to wait for all credential types processing to finish, before moving to next steps
        * adds finishImport function that should resume the import in respective importers once all conflicts were resolved (if there were any)

        Additionally adds a small change to trigger the UI transition from not
        started to importing in credential_import_mediator.
        Bug: 450982128
        Change-Id: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
        Commit-Queue: Rafał Godlewski <rg...@google.com>
        Reviewed-by: Tommy Martino <tmar...@chromium.org>
        Cr-Commit-Position: refs/heads/main@{#1544230}
        Files:
        Change size: M
        Delta: 6 files changed, 148 insertions(+), 28 deletions(-)
        Branch: refs/heads/main
        Submit Requirements:
        • requirement satisfiedCode-Review: +1 by Tommy Martino
        Open in Gerrit
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: merged
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I1ef7451713835fbc1a0d3d8c37eb29479f962f0c
        Gerrit-Change-Number: 7132421
        Gerrit-PatchSet: 13
        Gerrit-Owner: Rafał Godlewski <rg...@google.com>
        Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
        Gerrit-Reviewer: Rafał Godlewski <rg...@google.com>
        Gerrit-Reviewer: Tommy Martino <tmar...@chromium.org>
        open
        diffy
        satisfied_requirement
        Reply all
        Reply to author
        Forward
        0 new messages