Use SF Symbol (custom password symbol) for password icons. [chromium/src : main]

Huiting Yu (Gerrit)

unread,

Sep 19, 2022, 3:10:07 PM9/19/22

to feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, chromium...@chromium.org

Set Ready For Review

View Change

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Huiting Yu (Gerrit)

unread,

Sep 19, 2022, 3:10:58 PM9/19/22

to Gauthier Ambard, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention is currently required from: Gauthier Ambard.

Huiting Yu would like Gauthier Ambard to review this change.

View Change

Use SF Symbol (custom password symbol) for password icons.

The SF symbol will be used for:
1. save & update password icon in the message banner.
2. save & update password icon in the badge overflow menu.

Change-Id: Icd13b46b18375d846b6f9dbee3a239572045d919
---
M ios/chrome/browser/ui/badges/BUILD.gn
M ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator_unittest.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator_unittest.mm
7 files changed, 87 insertions(+), 17 deletions(-)

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Gauthier Ambard (Gerrit)

unread,

Sep 20, 2022, 5:33:19 AM9/20/22

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

Attention is currently required from: Huiting Yu.

View Change

7 comments:

Commit Message:
- Patch Set #3, Line 12:
  Same as the other CL.
File ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm:
- Patch Set #3, Line 27: // The image used for password related badges
  Comments end with a period.
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn:
- Patch Set #3, Line 56: "//ios/chrome/browser/ui/icons:symbols",
  Those are duplicated, remove duplicates.
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm:
- Patch Set #3, Line 70: - (UIImage*)iconImageWithConfig:
  Add a comment.
- Patch Set #3, Line 71: (SavePasswordInfobarBannerOverlayRequestConfig*)config {
  This method should probably be in the main implementation file, under a `#pragma - Private` section.
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm:
- Patch Set #3, Line 69: - (UIImage*)iconImageWithConfig:
  Add a comment.
- Patch Set #3, Line 70: (UpdatePasswordInfobarBannerOverlayRequestConfig*)config {
  This method should probably be in the main implementation file, under a `#pragma - Private` section.

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Huiting Yu (Gerrit)

unread,

Sep 20, 2022, 10:29:55 PM9/20/22

to feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention is currently required from: Huiting Yu.

Huiting Yu uploaded patch set #4 to this change.

View Change

Use SF Symbol (custom password symbol) for password icons.

The SF symbol will be used for:
1. save & update password icon in the message banner.
2. save & update password icon in the badge overflow menu.

Change-Id: Icd13b46b18375d846b6f9dbee3a239572045d919

Bug: 1315544


---
M ios/chrome/browser/ui/badges/BUILD.gn
M ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator_unittest.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator_unittest.mm

7 files changed, 89 insertions(+), 17 deletions(-)

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Huiting Yu (Gerrit)

unread,

Sep 20, 2022, 10:53:23 PM9/20/22

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

Attention is currently required from: Gauthier Ambard.

Patch set 5:Commit-Queue +1

View Change

7 comments:

Commit Message:

- Patch Set #3, Line 12:
  Same as the other CL.

- Done
File ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm:

- Patch Set #3, Line 27: // The image used for password related badges
  Comments end with a period.

- Done
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn:

- Patch Set #3, Line 56: "//ios/chrome/browser/ui/icons:symbols",
  Those are duplicated, remove duplicates.

- Done
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm:

- Patch Set #3, Line 70: - (UIImage*)iconImageWithConfig:
  Add a comment.

- Done

- Patch Set #3, Line 71: (SavePasswordInfobarBannerOverlayRequestConfig*)config {
  This method should probably be in the main implementation file, under a `#pragma - Private` section.

- Done
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm:

- Patch Set #3, Line 69: - (UIImage*)iconImageWithConfig:
  Add a comment.

- Done

- Patch Set #3, Line 70: (UpdatePasswordInfobarBannerOverlayRequestConfig*)config {
  This method should probably be in the main implementation file, under a `#pragma - Private` section.

- Done

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Findit (Gerrit)

unread,

Sep 20, 2022, 11:45:56 PM9/20/22

to Huiting Yu, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Gauthier Ambard, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Gauthier Ambard.

View Change

1 comment:

File ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm:
- Robot Comment from Chromium Coverage Checker (run ID chromium/src~3906221~5):
  Patch Set #5:
  Incremental Coverage (All Tests) for this file is below 80 %. Please add tests for uncovered lines.

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Findit (Gerrit)

unread,

Sep 20, 2022, 11:53:30 PM9/20/22

to Huiting Yu, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Gauthier Ambard, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Gauthier Ambard, Huiting Yu.

View Change

1 comment:

File ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm:
- Robot Comment from Chromium Coverage Checker (run ID chromium/src~3906221~5):
  Patch Set #5:
  Incremental Coverage (All Tests) for this file is below 80 %. Please add tests for uncovered lines.

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Gauthier Ambard (Gerrit)

unread,

Sep 21, 2022, 9:22:07 AM9/21/22

to Huiting Yu, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Findit, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Huiting Yu.

View Change

4 comments:

Commit Message:
- Patch Set #5, Line 15: Bug: 1315544
  Nit: The bug number is supposed to be before the `Change-Id` (you can see that it has been added below your bug number here as Change-Id is always the last line).
  You can remove the Change-Id line on line 13.
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn:
- Patch Set #5, Line 54: "//ios/chrome/browser/ui/icons:infobar_icons",
  This one is also a duplicate
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm:
- Patch Set #5, Line 70: #pragma mark - Private
  This is still in the `ConsumerSupport` class extension. I think it would make more sense to have it in the main implementation of the class no?
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm:
- Patch Set #5, Line 69: #pragma mark - Private
  Same

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Huiting Yu (Gerrit)

unread,

Sep 21, 2022, 11:30:25 AM9/21/22

to feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention is currently required from: Huiting Yu.

Huiting Yu uploaded patch set #6 to this change.

View Change

Use SF Symbol (custom password symbol) for password icons.

The SF symbol will be used for:
1. save & update password icon in the message banner.
2. save & update password icon in the badge overflow menu.

Bug: 1315544
Change-Id: Icd13b46b18375d846b6f9dbee3a239572045d919


---
M ios/chrome/browser/ui/badges/BUILD.gn
M ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator_unittest.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator_unittest.mm

7 files changed, 93 insertions(+), 17 deletions(-)

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Huiting Yu (Gerrit)

unread,

Sep 21, 2022, 12:04:36 PM9/21/22

to feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Findit, Gauthier Ambard, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Gauthier Ambard.

View Change

4 comments:

Commit Message:

- Patch Set #5, Line 15: Bug: 1315544

- Nit: The bug number is supposed to be before the `Change-Id` (you can see that it has been added bel […]
  Done
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn:

- Patch Set #5, Line 54: "//ios/chrome/browser/ui/icons:infobar_icons",
  This one is also a duplicate

- Done
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm:

- Patch Set #5, Line 70: #pragma mark - Private

- This is still in the `ConsumerSupport` class extension. […]
  Done. Thanks for explaining, i haven't got used to the Objc syntax.
File ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm:

- Patch Set #5, Line 69: #pragma mark - Private
  Same

- Done

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Findit (Gerrit)

unread,

Sep 21, 2022, 1:53:00 PM9/21/22

to Huiting Yu, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Gauthier Ambard, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Gauthier Ambard.

View Change

1 comment:

File ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm:
- Robot Comment from Chromium Coverage Checker (run ID chromium/src~3906221~7):
  Patch Set #7:

- Incremental Coverage (All Tests) for this file is below 80 %. Please add tests for uncovered lines.

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Findit (Gerrit)

unread,

Sep 21, 2022, 2:46:46 PM9/21/22

to Huiting Yu, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Gauthier Ambard, Chromium LUCI CQ, chromium...@chromium.org

Gauthier Ambard (Gerrit)

unread,

Sep 22, 2022, 4:16:05 AM9/22/22

to Huiting Yu, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Findit, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Huiting Yu.

Patch set 7:Code-Review +1

View Change

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Huiting Yu (Gerrit)

unread,

Sep 22, 2022, 11:58:44 AM9/22/22

to feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Gauthier Ambard, Findit, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Huiting Yu.

Patch set 7:Commit-Queue +2

View Change

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Sep 22, 2022, 12:01:41 PM9/22/22

to Huiting Yu, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, Gauthier Ambard, Findit, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Huiting Yu: Commit
  Gauthier Ambard: Looks good to me

Use SF Symbol (custom password symbol) for password icons.

The SF symbol will be used for:
1. save & update password icon in the message banner.
2. save & update password icon in the badge overflow menu.

Bug: 1315544
Change-Id: Icd13b46b18375d846b6f9dbee3a239572045d919

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3906221
Reviewed-by: Gauthier Ambard <gam...@chromium.org>
Commit-Queue: Huiting Yu <huit...@google.com>
Cr-Commit-Position: refs/heads/main@{#1050215}


---
M ios/chrome/browser/ui/badges/BUILD.gn
M ios/chrome/browser/ui/badges/badge_overflow_menu_util.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/BUILD.gn
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/save_password_infobar_banner_overlay_mediator_unittest.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator.mm
M ios/chrome/browser/ui/overlays/infobar_banner/passwords/update_password_infobar_banner_overlay_mediator_unittest.mm

7 files changed, 96 insertions(+), 17 deletions(-)

To view, visit change 3906221. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward