[iOS] Move actor/tools into the actor:: namespace [chromium/src : main]

0 views
Skip to first unread message

Nicolas MacBeth (Gerrit)

unread,
Apr 2, 2026, 2:25:48 PM (3 days ago) Apr 2
to Kirubel Aklilu, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Kirubel Aklilu

Nicolas MacBeth voted and added 1 comment

Votes added by Nicolas MacBeth

Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Nicolas MacBeth . resolved

ptal, thanks!

Open in Gerrit

Related details

Attention is currently required from:
  • Kirubel Aklilu
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: Ie47b3218750d4424aede5ba9705f622190eae0b2
Gerrit-Change-Number: 7726608
Gerrit-PatchSet: 2
Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
Gerrit-Attention: Kirubel Aklilu <kak...@chromium.org>
Gerrit-Comment-Date: Thu, 02 Apr 2026 18:25:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Kirubel Aklilu (Gerrit)

unread,
Apr 2, 2026, 2:56:41 PM (3 days ago) Apr 2
to Nicolas MacBeth, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Nicolas MacBeth

Kirubel Aklilu voted and added 6 comments

Votes added by Kirubel Aklilu

Code-Review+1

6 comments

Patchset-level comments
Kirubel Aklilu . resolved

LGTM, with some nits

File ios/chrome/browser/intelligence/actor/model/actor_service.h
Line 24, Patchset 2 (Latest):} // namespace actor
Kirubel Aklilu . unresolved

Why not put this whole class in the `actor` namespace? Then this and the .mm file can avoid qualifying with the namespace throughout.

File ios/chrome/browser/intelligence/actor/model/actor_service_unittest.mm
Line 20, Patchset 2 (Latest):class ActorServiceTest : public PlatformTest {
Kirubel Aklilu . unresolved

nit: can also be pulled into actor namespace

File ios/chrome/browser/intelligence/actor/model/aggregated_journal_unittest.mm
Line 25, Patchset 2 (Latest): ActorTaskId task_id(base::Token(0, 1));
Kirubel Aklilu . unresolved

nit: can this just be the default ctor? I picked 1 arbitrarily here and it has no behavioral impact iirc. Same throughout.

File ios/chrome/browser/intelligence/actor/tools/model/actor_tool.h
Line 28, Patchset 2 (Latest): using ToolExecutionResult = base::expected<void, ActorToolError>;
Kirubel Aklilu . resolved

Ideally this change is in a separate CL, but seems fine to me since you're updating most callsites already.

File ios/chrome/browser/intelligence/actor/tools/model/click_tool_java_script_feature.h
Line 34, Patchset 2 (Latest): actor::ActorTool::ToolExecutionCallback callback);
Kirubel Aklilu . unresolved

nit: can be dropped now

Open in Gerrit

Related details

Attention is currently required from:
  • Nicolas MacBeth
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • 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: Ie47b3218750d4424aede5ba9705f622190eae0b2
    Gerrit-Change-Number: 7726608
    Gerrit-PatchSet: 2
    Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
    Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
    Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
    Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>
    Gerrit-Comment-Date: Thu, 02 Apr 2026 18:56:34 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Nicolas MacBeth (Gerrit)

    unread,
    Apr 2, 2026, 7:36:10 PM (3 days ago) Apr 2
    to Kirubel Aklilu, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
    Attention needed from Kirubel Aklilu

    Nicolas MacBeth added 5 comments

    Patchset-level comments
    File-level comment, Patchset 4 (Latest):
    Nicolas MacBeth . resolved

    thanks!

    File ios/chrome/browser/intelligence/actor/model/actor_service.h
    Line 24, Patchset 2:} // namespace actor
    Kirubel Aklilu . resolved

    Why not put this whole class in the `actor` namespace? Then this and the .mm file can avoid qualifying with the namespace throughout.

    Nicolas MacBeth

    yeah, feels weird to me too. I thought keeping the Service only outside of `actor::` made sense for some reason, but will put in there.

    File ios/chrome/browser/intelligence/actor/model/actor_service_unittest.mm
    Line 20, Patchset 2:class ActorServiceTest : public PlatformTest {
    Kirubel Aklilu . resolved

    nit: can also be pulled into actor namespace

    Nicolas MacBeth

    Done

    File ios/chrome/browser/intelligence/actor/model/aggregated_journal_unittest.mm
    Line 25, Patchset 2: ActorTaskId task_id(base::Token(0, 1));
    Kirubel Aklilu . resolved

    nit: can this just be the default ctor? I picked 1 arbitrarily here and it has no behavioral impact iirc. Same throughout.

    Nicolas MacBeth

    Done

    File ios/chrome/browser/intelligence/actor/tools/model/click_tool_java_script_feature.h
    Line 34, Patchset 2: actor::ActorTool::ToolExecutionCallback callback);
    Kirubel Aklilu . resolved

    nit: can be dropped now

    Nicolas MacBeth

    Done

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Kirubel Aklilu
    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: Ie47b3218750d4424aede5ba9705f622190eae0b2
      Gerrit-Change-Number: 7726608
      Gerrit-PatchSet: 4
      Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
      Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
      Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
      Gerrit-Attention: Kirubel Aklilu <kak...@chromium.org>
      Gerrit-Comment-Date: Thu, 02 Apr 2026 23:36:05 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Kirubel Aklilu <kak...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Kirubel Aklilu (Gerrit)

      unread,
      Apr 3, 2026, 10:29:18 AM (2 days ago) Apr 3
      to Nicolas MacBeth, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
      Attention needed from Nicolas MacBeth

      Kirubel Aklilu voted and added 1 comment

      Votes added by Kirubel Aklilu

      Code-Review+1

      1 comment

      File ios/chrome/browser/profile/model/keyed_service_factories.mm
      Line 220, Patchset 4 (Latest): // Keep this list alphabetized -- namespaced factories first, followed by
      // non-namespaced factories.
      Kirubel Aklilu . unresolved

      Looks like we need to pull the `actor::ActorServiceFactory` up here

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Nicolas MacBeth
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • 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: Ie47b3218750d4424aede5ba9705f622190eae0b2
        Gerrit-Change-Number: 7726608
        Gerrit-PatchSet: 4
        Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
        Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
        Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
        Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>
        Gerrit-Comment-Date: Fri, 03 Apr 2026 14:29:11 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Nicolas MacBeth (Gerrit)

        unread,
        Apr 3, 2026, 11:37:02 AM (2 days ago) Apr 3
        to Rohit Rao, Kirubel Aklilu, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
        Attention needed from Rohit Rao

        Nicolas MacBeth added 2 comments

        Patchset-level comments
        Nicolas MacBeth . resolved

        ptal, thanks!

        File ios/chrome/browser/profile/model/keyed_service_factories.mm
        Line 220, Patchset 4: // Keep this list alphabetized -- namespaced factories first, followed by
        // non-namespaced factories.
        Kirubel Aklilu . resolved

        Looks like we need to pull the `actor::ActorServiceFactory` up here

        Nicolas MacBeth

        Done

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Rohit Rao
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement is not 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: Ie47b3218750d4424aede5ba9705f622190eae0b2
          Gerrit-Change-Number: 7726608
          Gerrit-PatchSet: 4
          Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
          Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Rohit Rao <rohi...@chromium.org>
          Gerrit-Attention: Rohit Rao <rohi...@chromium.org>
          Gerrit-Comment-Date: Fri, 03 Apr 2026 15:36:53 +0000
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Rohit Rao (Gerrit)

          unread,
          Apr 3, 2026, 11:39:43 AM (2 days ago) Apr 3
          to Nicolas MacBeth, Rohit Rao, Kirubel Aklilu, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
          Attention needed from Nicolas MacBeth

          Rohit Rao voted and added 1 comment

          Votes added by Rohit Rao

          Code-Review+1

          1 comment

          Patchset-level comments
          File-level comment, Patchset 5 (Latest):
          Rohit Rao . resolved

          profile and web lgtm

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Nicolas MacBeth
          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: Ie47b3218750d4424aede5ba9705f622190eae0b2
          Gerrit-Change-Number: 7726608
          Gerrit-PatchSet: 5
          Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
          Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Rohit Rao <rohi...@chromium.org>
          Gerrit-Attention: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Comment-Date: Fri, 03 Apr 2026 15:39:33 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Nicolas MacBeth (Gerrit)

          unread,
          Apr 3, 2026, 11:45:08 AM (2 days ago) Apr 3
          to Rohit Rao, Kirubel Aklilu, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

          Nicolas MacBeth voted Commit-Queue+1

          Commit-Queue+1
          Open in Gerrit

          Related details

          Attention set is empty
          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: Ie47b3218750d4424aede5ba9705f622190eae0b2
          Gerrit-Change-Number: 7726608
          Gerrit-PatchSet: 5
          Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
          Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Rohit Rao <rohi...@chromium.org>
          Gerrit-Comment-Date: Fri, 03 Apr 2026 15:45:00 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Nicolas MacBeth (Gerrit)

          unread,
          Apr 3, 2026, 11:46:20 AM (2 days ago) Apr 3
          to Rohit Rao, Kirubel Aklilu, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

          Nicolas MacBeth voted Commit-Queue+0

          Commit-Queue+0
          Open in Gerrit

          Related details

          Attention set is empty
          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: Ie47b3218750d4424aede5ba9705f622190eae0b2
          Gerrit-Change-Number: 7726608
          Gerrit-PatchSet: 5
          Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
          Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Rohit Rao <rohi...@chromium.org>
          Gerrit-Comment-Date: Fri, 03 Apr 2026 15:46:10 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Nicolas MacBeth (Gerrit)

          unread,
          Apr 3, 2026, 11:47:06 AM (2 days ago) Apr 3
          to Rohit Rao, Kirubel Aklilu, Chromium LUCI CQ, chromium...@chromium.org, bling-ai-found...@google.com, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

          Nicolas MacBeth voted Commit-Queue+2

          Commit-Queue+2
          Open in Gerrit

          Related details

          Attention set is empty
          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: Ie47b3218750d4424aede5ba9705f622190eae0b2
          Gerrit-Change-Number: 7726608
          Gerrit-PatchSet: 5
          Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
          Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Rohit Rao <rohi...@chromium.org>
          Gerrit-Comment-Date: Fri, 03 Apr 2026 15:46:59 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Chromium LUCI CQ (Gerrit)

          unread,
          Apr 3, 2026, 12:31:38 PM (2 days ago) Apr 3
          to Nicolas MacBeth, Rohit Rao, Kirubel Aklilu, chromium...@chromium.org, bling-ai-found...@google.com, 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] Move actor/tools into the actor:: namespace

          Moved tools into the actor:: namespace to be consistent with the actor
          model. Also rename the tool execution result to reduce the "actor"
          verbosity when chaining namespaces, since it's defined in the ActorTool
          class. Finally removed a redundant ActorEngine callback
          Fixed: 499003982
          Change-Id: Ie47b3218750d4424aede5ba9705f622190eae0b2
          Reviewed-by: Kirubel Aklilu <kak...@chromium.org>
          Commit-Queue: Nicolas MacBeth <nicolas...@google.com>
          Reviewed-by: Rohit Rao <rohi...@chromium.org>
          Cr-Commit-Position: refs/heads/main@{#1609820}
          Files:
          Change size: L
          Delta: 54 files changed, 359 insertions(+), 179 deletions(-)
          Branch: refs/heads/main
          Submit Requirements:
          • requirement satisfiedCode-Review: +1 by Rohit Rao, +1 by Kirubel Aklilu
          Open in Gerrit
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: merged
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Ie47b3218750d4424aede5ba9705f622190eae0b2
          Gerrit-Change-Number: 7726608
          Gerrit-PatchSet: 6
          Gerrit-Owner: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
          Gerrit-Reviewer: Kirubel Aklilu <kak...@chromium.org>
          Gerrit-Reviewer: Nicolas MacBeth <nicolas...@google.com>
          Gerrit-Reviewer: Rohit Rao <rohi...@chromium.org>
          open
          diffy
          satisfied_requirement
          Reply all
          Reply to author
          Forward
          0 new messages