Tommy Martino (Gerrit)

unread,

Nov 10, 2025, 5:28:45 PM (2 days ago) Nov 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:

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

Rafał Godlewski (Gerrit)

unread,

Nov 11, 2025, 9:31:06 AM (yesterday) Nov 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:

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

Rafał Godlewski (Gerrit)

unread,

10:59 AM (7 hours ago) 10:59 AM

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:

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,

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

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:

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

Reply all

Reply to author

Forward

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

Tommy Martino (Gerrit)

Tommy Martino added 5 comments

Related details

Rafał Godlewski (Gerrit)

Rafał Godlewski added 4 comments

Related details

Rafał Godlewski (Gerrit)

Rafał Godlewski added 1 comment

Related details

Tommy Martino (Gerrit)

Tommy Martino voted and added 1 comment

Votes added by Tommy Martino

1 comment

Related details