Alexis Hétu (Gerrit)

unread,

1:21 PM (6 hours ago) 1:21 PM

to Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Leo Zhao

Alexis Hétu added 5 comments

Commit Message

Line 12, Patchset 2 (Latest):a country field, after the default text "_" and before the disclosure

Alexis Hétu . unresolved

if it `_` or `-`?

File ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_country_item.mm

Line 62, Patchset 2 (Latest):// exposed in the header. If that is exposed in the future, this method can be

Alexis Hétu . unresolved

This sentence has no ending.

File ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_edit_table_view_controller.mm

Line 205, Patchset 2 (Latest):

    AutofillAIEntityCountryItem* countryItem =
        base::apple::ObjCCastStrict<AutofillAIEntityCountryItem>(item);
    if (self.tableView.editing) {
      countryItem.accessoryType = UITableViewCellAccessoryDisclosureIndicator;
      countryItem.editingAccessoryType =
          UITableViewCellAccessoryDisclosureIndicator;
      countryItem.selectionStyle = UITableViewCellSelectionStyleDefault;
    } else {
      countryItem.accessoryType = UITableViewCellAccessoryNone;
      countryItem.editingAccessoryType = UITableViewCellAccessoryNone;
      countryItem.selectionStyle = UITableViewCellSelectionStyleNone;
    }
  }

Alexis Hétu . unresolved

Duplicated code, move this to a utility function.

Line 447, Patchset 2 (Latest): bool isEmpty = countryItem.detailText.length <= 1;

Alexis Hétu . unresolved

Optional nit: We could do a check that's a bit more robust, to make sure that if a single character is entered by the user, it's treated differently that the "empty" scenario.
```suggestion
  BOOL isEmpty = (countryItem.detailText.length == 0) || 
                 [countryItem.detailText isEqualToString:@"-"];
```

Line 448, Patchset 2 (Latest): bool isRequired =

Alexis Hétu . unresolved

BOOL

Open in Gerrit

Related details

Attention is currently required from:

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,

4:02 PM (3 hours ago) 4:02 PM

to Chromium LUCI CQ, Alexis Hétu, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Alexis Hétu

Leo Zhao added 4 comments

Commit Message

Line 12, Patchset 2:a country field, after the default text "_" and before the disclosure

Alexis Hétu . unresolved

if it `_` or `-`?

Leo Zhao

When country attribute is set to an empty string, the `GetInfo` returns "_".

File ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_country_item.mm

Line 62, Patchset 2:// exposed in the header. If that is exposed in the future, this method can be

Alexis Hétu . resolved

This sentence has no ending.

Leo Zhao

I was thinking about adding a note here to that later on when its parent class is updated to expose a private method. But then that is not likely to happen. So, I have removed this comment. Just leave the necessary comment about the functionality of this method.

File ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_edit_table_view_controller.mm

Line 205, Patchset 2: AutofillAIEntityCountryItem* countryItem =


        base::apple::ObjCCastStrict<AutofillAIEntityCountryItem>(item);
    if (self.tableView.editing) {
      countryItem.accessoryType = UITableViewCellAccessoryDisclosureIndicator;
      countryItem.editingAccessoryType =
          UITableViewCellAccessoryDisclosureIndicator;
      countryItem.selectionStyle = UITableViewCellSelectionStyleDefault;
    } else {
      countryItem.accessoryType = UITableViewCellAccessoryNone;
      countryItem.editingAccessoryType = UITableViewCellAccessoryNone;
      countryItem.selectionStyle = UITableViewCellSelectionStyleNone;
    }
  }

Alexis Hétu . resolved

Duplicated code, move this to a utility function.

Leo Zhao

Done.

Line 447, Patchset 2: bool isEmpty = countryItem.detailText.length <= 1;

Alexis Hétu . resolved

Optional nit: We could do a check that's a bit more robust, to make sure that if a single character is entered by the user, it's treated differently that the "empty" scenario.
```suggestion
  BOOL isEmpty = (countryItem.detailText.length == 0) || 
                 [countryItem.detailText isEqualToString:@"-"];
```

Leo Zhao

Users can not edit this field directly. They have to pick a country from a list of countries. No countries are shorter than 4 characters from scrolling through the countries from the list.

Open in Gerrit

Related details

Attention is currently required from:

Alexis Hétu

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,

4:03 PM (3 hours ago) 4:03 PM

to Chromium LUCI CQ, Alexis Hétu, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Alexis Hétu

Leo Zhao added 2 comments

Commit Message

Line 12, Patchset 2:a country field, after the default text "_" and before the disclosure

Alexis Hétu . resolved

if it `_` or `-`?

Leo Zhao

When country attribute is set to an empty string, the `GetInfo` returns "_".

Leo Zhao

Done

File ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_edit_table_view_controller.mm

Line 448, Patchset 2: bool isRequired =

Alexis Hétu . resolved

BOOL

Leo Zhao

Updated.

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,

4:15 PM (3 hours ago) 4:15 PM

to Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Leo Zhao

Alexis Hétu voted and added 1 comment

Votes added by Alexis Hétu

Code-Review

+1

1 comment

File ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_edit_table_view_controller.mm

Line 447, Patchset 2: bool isEmpty = countryItem.detailText.length <= 1;

Alexis Hétu . resolved

Optional nit: We could do a check that's a bit more robust, to make sure that if a single character is entered by the user, it's treated differently that the "empty" scenario.
```suggestion
  BOOL isEmpty = (countryItem.detailText.length == 0) || 
                 [countryItem.detailText isEqualToString:@"-"];
```

Leo Zhao

Users can not edit this field directly. They have to pick a country from a list of countries. No countries are shorter than 4 characters from scrolling through the countries from the list.

Alexis Hétu

Not in the country selector, but I think "United States" comes out as "US" in this string, for example. Anyway, not critical at the moment.

Open in Gerrit

Related details

Attention is currently required from:

Leo Zhao

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

open

diffy

Leo Zhao (Gerrit)

unread,

4:53 PM (3 hours ago) 4:53 PM

to Alexis Hétu, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Leo Zhao voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

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

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

5:06 PM (2 hours ago) 5:06 PM

to Leo Zhao, Alexis Hétu, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[iOS][Forms AI] Error state for country field

This CL adds error state to country field. Since "Add payment" does not
have country field, entity edit has to create an error state for
country field. In this case, The same red circle symbol is used for


a country field, after the default text "_" and before the disclosure

icon.

Screenshots:
https://screenshot.googleplex.com/6CCbg7VBtpearMF

Bug: 480933727

Change-Id: Ia0968fdc31495112d8aeeda5f46e4ccb5699bd3c

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7726762

Reviewed-by: Alexis Hétu <su...@chromium.org>

Commit-Queue: Leo Zhao <leo...@google.com>

Cr-Commit-Position: refs/heads/main@{#1609488}

Files:

M ios/chrome/browser/settings/autofill/autofill_ai/coordinator/autofill_ai_entity_edit_mediator.mm
M ios/chrome/browser/settings/autofill/autofill_ai/ui/BUILD.gn
M ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_country_item.mm
M ios/chrome/browser/settings/autofill/autofill_ai/ui/autofill_ai_entity_edit_table_view_controller.mm

Change size: M

Delta: 4 files changed, 96 insertions(+), 18 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Alexis Hétu

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[iOS][Forms AI] Error state for country field [chromium/src : main]

Alexis Hétu (Gerrit)

Alexis Hétu added 5 comments

Related details

Leo Zhao (Gerrit)

Leo Zhao added 4 comments

Related details

Leo Zhao (Gerrit)

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

Leo Zhao (Gerrit)

Leo Zhao voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information