Add actions to Gemini dynamic settings [chromium/src : main]

0 views
Skip to first unread message

Joe Peplowski (Gerrit)

unread,
Dec 23, 2025, 11:23:12 AM (4 days ago) Dec 23
to Adam Arcaro, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Adam Arcaro

Joe Peplowski voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Adam Arcaro
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not 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: Ic0183f40a7bd0b8fb886af3d6a9f0d6bf4ad56ed
Gerrit-Change-Number: 7281255
Gerrit-PatchSet: 5
Gerrit-Owner: Joe Peplowski <joepep...@google.com>
Gerrit-Reviewer: Adam Arcaro <ada...@google.com>
Gerrit-Reviewer: Joe Peplowski <joepep...@google.com>
Gerrit-Attention: Adam Arcaro <ada...@google.com>
Gerrit-Comment-Date: Tue, 23 Dec 2025 16:23:08 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Adam Arcaro (Gerrit)

unread,
Dec 23, 2025, 1:06:23 PM (4 days ago) Dec 23
to Joe Peplowski, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Joe Peplowski

Adam Arcaro added 2 comments

Patchset-level comments
File-level comment, Patchset 5 (Latest):
Adam Arcaro . resolved

Love this approach! Just see one comment but otherwise lgtm

File ios/chrome/browser/settings/ui_bundled/bwg/model/gemini_dynamic_settings_item.mm
Line 26, Patchset 5 (Latest): } else {
Adam Arcaro . unresolved

I think we should be more explicit about the types here. `UITableViewCellAccessoryDisclosureIndicator` will present a chevron which implies some navigability. This is fine for `GeminiSettingsActionTypeViewController` but we shouldn't assume it's what we want for other enum values

How about something like
```
switch (action.type) {
  case GeminiSettingsActionTypeURL: {
    self.accessorySymbol = TableViewDetailTextCellAccessorySymbolExternalLink;
    self.accessibilityTraits |= UIAccessibilityTraitLink;
    break;
  }
  case GeminiSettingsActionTypeViewController: {
    self.accessoryType = UITableViewCellAccessoryDisclosureIndicator;
    self.accessibilityTraits |= UIAccessibilityTraitButton;
    break;
  }
  case GeminiSettingsActionTypeUnknown: {
    self.accessoryType = UITableViewCellAccessoryNone;
    break;
  }
}
```
Open in Gerrit

Related details

Attention is currently required from:
  • Joe Peplowski
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not 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: Ic0183f40a7bd0b8fb886af3d6a9f0d6bf4ad56ed
    Gerrit-Change-Number: 7281255
    Gerrit-PatchSet: 5
    Gerrit-Owner: Joe Peplowski <joepep...@google.com>
    Gerrit-Reviewer: Adam Arcaro <ada...@google.com>
    Gerrit-Reviewer: Joe Peplowski <joepep...@google.com>
    Gerrit-Attention: Joe Peplowski <joepep...@google.com>
    Gerrit-Comment-Date: Tue, 23 Dec 2025 18:06:17 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Joe Peplowski (Gerrit)

    unread,
    Dec 23, 2025, 1:52:24 PM (4 days ago) Dec 23
    to Adam Arcaro, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
    Attention needed from Adam Arcaro

    Joe Peplowski voted and added 1 comment

    Votes added by Joe Peplowski

    Commit-Queue+1

    1 comment

    File ios/chrome/browser/settings/ui_bundled/bwg/model/gemini_dynamic_settings_item.mm
    Line 26, Patchset 5: } else {
    Adam Arcaro . resolved

    I think we should be more explicit about the types here. `UITableViewCellAccessoryDisclosureIndicator` will present a chevron which implies some navigability. This is fine for `GeminiSettingsActionTypeViewController` but we shouldn't assume it's what we want for other enum values

    How about something like
    ```
    switch (action.type) {
      case GeminiSettingsActionTypeURL: {
        self.accessorySymbol = TableViewDetailTextCellAccessorySymbolExternalLink;
        self.accessibilityTraits |= UIAccessibilityTraitLink;
        break;
      }
      case GeminiSettingsActionTypeViewController: {
        self.accessoryType = UITableViewCellAccessoryDisclosureIndicator;
        self.accessibilityTraits |= UIAccessibilityTraitButton;
        break;
      }
      case GeminiSettingsActionTypeUnknown: {
        self.accessoryType = UITableViewCellAccessoryNone;
        break;
      }
    }
    ```
    Joe Peplowski

    Sounds great! Done.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Adam Arcaro
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not 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: Ic0183f40a7bd0b8fb886af3d6a9f0d6bf4ad56ed
      Gerrit-Change-Number: 7281255
      Gerrit-PatchSet: 7
      Gerrit-Owner: Joe Peplowski <joepep...@google.com>
      Gerrit-Reviewer: Adam Arcaro <ada...@google.com>
      Gerrit-Reviewer: Joe Peplowski <joepep...@google.com>
      Gerrit-Attention: Adam Arcaro <ada...@google.com>
      Gerrit-Comment-Date: Tue, 23 Dec 2025 18:52:19 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Adam Arcaro <ada...@google.com>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Adam Arcaro (Gerrit)

      unread,
      Dec 23, 2025, 2:40:51 PM (4 days ago) Dec 23
      to Joe Peplowski, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
      Attention needed from Joe Peplowski

      Adam Arcaro voted and added 1 comment

      Votes added by Adam Arcaro

      Code-Review+1

      1 comment

      Patchset-level comments
      File-level comment, Patchset 7 (Latest):
      Adam Arcaro . resolved

      Thanks Joe!

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Joe Peplowski
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement is not 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: Ic0183f40a7bd0b8fb886af3d6a9f0d6bf4ad56ed
      Gerrit-Change-Number: 7281255
      Gerrit-PatchSet: 7
      Gerrit-Owner: Joe Peplowski <joepep...@google.com>
      Gerrit-Reviewer: Adam Arcaro <ada...@google.com>
      Gerrit-Reviewer: Joe Peplowski <joepep...@google.com>
      Gerrit-Attention: Joe Peplowski <joepep...@google.com>
      Gerrit-Comment-Date: Tue, 23 Dec 2025 19:40:40 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Joemer Ramos (Gerrit)

      unread,
      Dec 26, 2025, 9:19:59 AM (yesterday) Dec 26
      to Joe Peplowski, Joemer Ramos, Adam Arcaro, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
      Attention needed from Joe Peplowski

      Joemer Ramos voted and added 1 comment

      Votes added by Joemer Ramos

      Code-Review+1

      1 comment

      Patchset-level comments
      Joemer Ramos . resolved

      lgtm

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Joe Peplowski
      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: Ic0183f40a7bd0b8fb886af3d6a9f0d6bf4ad56ed
        Gerrit-Change-Number: 7281255
        Gerrit-PatchSet: 7
        Gerrit-Owner: Joe Peplowski <joepep...@google.com>
        Gerrit-Reviewer: Adam Arcaro <ada...@google.com>
        Gerrit-Reviewer: Joe Peplowski <joepep...@google.com>
        Gerrit-Reviewer: Joemer Ramos <joeme...@google.com>
        Gerrit-Attention: Joe Peplowski <joepep...@google.com>
        Gerrit-Comment-Date: Fri, 26 Dec 2025 14:19:48 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        open
        diffy
        Reply all
        Reply to author
        Forward
        0 new messages