[iOS][Forms AI] add "Other" section into Addresses and more [chromium/src : main]

1 view

Skip to first unread message

Tommy Martino (Gerrit)

unread,

11:29 AM (10 hours ago) 11:29 AM

to Leo Zhao, Chromium LUCI CQ, Alexis Hétu, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, srahim...@chromium.org, tmartino+tran...@chromium.org

Attention needed from Alexis Hétu and Leo Zhao

Tommy Martino added 2 comments

File ios/chrome/browser/settings/ui_bundled/autofill/autofill_profile_table_view_controller.mm

Line 410, Patchset 2 (Latest):

  if (!other.empty()) {
    [model addSectionWithIdentifier:SectionIdentifierOther];
    [model setHeader:[self otherSectionHeader]
        forSectionWithIdentifier:SectionIdentifierOther];

    std::vector<autofill::EntityLabel> labels = autofill::GetLabelsForEntities(
        other, /*attribute_types_to_ignore=*/{},
        /*only_disambiguating_types=*/true, /*obfuscate_sensitive_types=*/true,
        locale);

    for (size_t i = 0; i < other.size(); ++i) {
      [model addItem:[self itemForEntityInstance:*other[i]
                                       withLabel:labels[i]
                                            type:ItemTypeOther]
          toSectionWithIdentifier:SectionIdentifierOther];
    }
  }

Tommy Martino . unresolved

At this point it might be worth considering adding a helper method to construct these sections. You could take a `std::vector<const autofill::EntityInstance*>`, item type, and a section identifier as parameters.

Line 451, Patchset 2 (Latest):

- (TableViewHeaderFooterItem*)identityDocsSectionHeader {
  TableViewTextHeaderFooterItem* header = [[TableViewTextHeaderFooterItem alloc]
      initWithType:ItemTypeIdentityDocHeader];
  header.text = l10n_util::GetNSString(IDS_AUTOFILL_IDENTITY_DOCS_TITLE);
  return header;
}

- (TableViewHeaderFooterItem*)travelSectionHeader {
  TableViewTextHeaderFooterItem* header =
      [[TableViewTextHeaderFooterItem alloc] initWithType:ItemTypeTravelHeader];
  header.text = l10n_util::GetNSString(IDS_AUTOFILL_TRAVEL_TITLE);
  return header;
}

- (TableViewHeaderFooterItem*)otherSectionHeader {
  TableViewTextHeaderFooterItem* header =
      [[TableViewTextHeaderFooterItem alloc] initWithType:ItemTypeOtherHeader];
  header.text = l10n_util::GetNSString(IDS_IOS_AUTOFILL_AI_OTHER_TITLE);
  return header;
}

Tommy Martino . unresolved

Similarly, and especially if you take my suggestion above, you could make these into a single method that generates the header item based on the item type.

Open in Gerrit

Related details

Attention is currently required from:

Alexis Hétu
Leo Zhao

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

Leo Zhao (Gerrit)

unread,

1:44 PM (8 hours ago) 1:44 PM

to Chromium LUCI CQ, Tommy Martino, Alexis Hétu, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, srahim...@chromium.org, tmartino+tran...@chromium.org

Attention needed from Alexis Hétu and Tommy Martino

Leo Zhao added 2 comments

File ios/chrome/browser/settings/ui_bundled/autofill/autofill_profile_table_view_controller.mm

Line 410, Patchset 2: if (!other.empty()) {


    [model addSectionWithIdentifier:SectionIdentifierOther];
    [model setHeader:[self otherSectionHeader]
        forSectionWithIdentifier:SectionIdentifierOther];

    std::vector<autofill::EntityLabel> labels = autofill::GetLabelsForEntities(
        other, /*attribute_types_to_ignore=*/{},
        /*only_disambiguating_types=*/true, /*obfuscate_sensitive_types=*/true,
        locale);

    for (size_t i = 0; i < other.size(); ++i) {
      [model addItem:[self itemForEntityInstance:*other[i]
                                       withLabel:labels[i]
                                            type:ItemTypeOther]
          toSectionWithIdentifier:SectionIdentifierOther];
    }
  }

Tommy Martino . resolved

At this point it might be worth considering adding a helper method to construct these sections. You could take a `std::vector<const autofill::EntityInstance*>`, item type, and a section identifier as parameters.

Leo Zhao

Done.

Line 451, Patchset 2:- (TableViewHeaderFooterItem*)identityDocsSectionHeader {


  TableViewTextHeaderFooterItem* header = [[TableViewTextHeaderFooterItem alloc]
      initWithType:ItemTypeIdentityDocHeader];
  header.text = l10n_util::GetNSString(IDS_AUTOFILL_IDENTITY_DOCS_TITLE);
  return header;
}

- (TableViewHeaderFooterItem*)travelSectionHeader {
  TableViewTextHeaderFooterItem* header =
      [[TableViewTextHeaderFooterItem alloc] initWithType:ItemTypeTravelHeader];
  header.text = l10n_util::GetNSString(IDS_AUTOFILL_TRAVEL_TITLE);
  return header;
}

- (TableViewHeaderFooterItem*)otherSectionHeader {
  TableViewTextHeaderFooterItem* header =
      [[TableViewTextHeaderFooterItem alloc] initWithType:ItemTypeOtherHeader];
  header.text = l10n_util::GetNSString(IDS_IOS_AUTOFILL_AI_OTHER_TITLE);
  return header;
}

Tommy Martino . resolved

Similarly, and especially if you take my suggestion above, you could make these into a single method that generates the header item based on the item type.

Leo Zhao

Removed.

Open in Gerrit

Related details

Attention is currently required from:

Alexis Hétu
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,

3:07 PM (7 hours ago) 3:07 PM

to Leo Zhao, Chromium LUCI CQ, Tommy Martino, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, srahim...@chromium.org, tmartino+tran...@chromium.org

Attention needed from Leo Zhao and Tommy Martino

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

Code-Review

1 comment

File ios/chrome/browser/settings/ui_bundled/autofill/autofill_profile_table_view_controller.mm

Line 1634, Patchset 5 (Latest):

  ItemType itemType;
  TableViewTextHeaderFooterItem* header;
  switch (sectionIdentifier) {
    case SectionIdentifierIdentityDocs:
      itemType = ItemTypeIdentityDoc;


      header = [[TableViewTextHeaderFooterItem alloc]
          initWithType:ItemTypeIdentityDocHeader];
      header.text = l10n_util::GetNSString(IDS_AUTOFILL_IDENTITY_DOCS_TITLE);

      break;
    case SectionIdentifierTravel:
      itemType = ItemTypeTravel;


      header = [[TableViewTextHeaderFooterItem alloc]
          initWithType:ItemTypeTravelHeader];
      header.text = l10n_util::GetNSString(IDS_AUTOFILL_TRAVEL_TITLE);

      break;
    case SectionIdentifierOther:
    default:
      itemType = ItemTypeOther;


      header = [[TableViewTextHeaderFooterItem alloc]
          initWithType:ItemTypeOtherHeader];
      header.text = l10n_util::GetNSString(IDS_IOS_AUTOFILL_AI_OTHER_TITLE);

      break;
  }

Alexis Hétu . unresolved

Optional nit: This function is a bit long and I think this code would be clearer with a few utility functions (I haven't compiled those, I'm just writing them here, so don't necessarily use them as is):

```
ItemType ItemTypeFromSectionIdentifier(SectionIdentifier sectionIdentifier) {
  switch (sectionIdentifier) {
    case SectionIdentifierIdentityDocs:
      return ItemTypeIdentityDoc;
    case SectionIdentifierTravel:
      return ItemTypeTravel;
    case SectionIdentifierOther:
    default:
      return ItemTypeOther;
  }
}
```

```
ItemType HeaderTypeForItemType(ItemType itemType) {
  switch (itemType) {
    case ItemTypeIdentityDoc:
      return ItemTypeIdentityDocHeader;
    case ItemTypeTravel:
      return ItemTypeTravelHeader;
    case ItemTypeOther:
    default:
      return ItemTypeOtherHeader;
  }
}
```

```
NSString* HeaderTextForItemType(ItemType itemType) {
  switch (itemType) {
    case ItemTypeIdentityDoc:
      return l10n_util::GetNSString(IDS_AUTOFILL_IDENTITY_DOCS_TITLE);
    case ItemTypeTravel:
      return l10n_util::GetNSString(IDS_AUTOFILL_TRAVEL_TITLE);
    case ItemTypeOther:
    default:
      return l10n_util::GetNSString(IDS_IOS_AUTOFILL_AI_OTHER_TITLE);
  }
}
```

And then the code here becomes:
```
ItemType itemType = ItemTypeFromSectionIdentifier(sectionIdentifier);


TableViewTextHeaderFooterItem* header = [[TableViewTextHeaderFooterItem alloc]

          initWithType:HeaderTypeForItemType(itemType)];
header.text = HeaderTextForItemType(itemType);
```

Open in Gerrit

Related details

Attention is currently required from:

Leo Zhao
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

Leo Zhao (Gerrit)

unread,

8:19 PM (1 hour ago) 8:19 PM

to Alexis Hétu, Chromium LUCI CQ, Tommy Martino, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, srahim...@chromium.org, tmartino+tran...@chromium.org

Attention needed from Tommy Martino

Leo Zhao added 1 comment

File ios/chrome/browser/settings/ui_bundled/autofill/autofill_profile_table_view_controller.mm

Line 1634, Patchset 5: ItemType itemType;


  TableViewTextHeaderFooterItem* header;
  switch (sectionIdentifier) {
    case SectionIdentifierIdentityDocs:
      itemType = ItemTypeIdentityDoc;
      header = [[TableViewTextHeaderFooterItem alloc]
          initWithType:ItemTypeIdentityDocHeader];
      header.text = l10n_util::GetNSString(IDS_AUTOFILL_IDENTITY_DOCS_TITLE);
      break;
    case SectionIdentifierTravel:
      itemType = ItemTypeTravel;
      header = [[TableViewTextHeaderFooterItem alloc]
          initWithType:ItemTypeTravelHeader];
      header.text = l10n_util::GetNSString(IDS_AUTOFILL_TRAVEL_TITLE);
      break;
    case SectionIdentifierOther:
    default:
      itemType = ItemTypeOther;
      header = [[TableViewTextHeaderFooterItem alloc]
          initWithType:ItemTypeOtherHeader];
      header.text = l10n_util::GetNSString(IDS_IOS_AUTOFILL_AI_OTHER_TITLE);
      break;
  }

Alexis Hétu . resolved

Leo Zhao

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

Reply all

Reply to author

Forward

0 new messages