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

0 views

Skip to first unread message

Nicolas MacBeth (Gerrit)

unread,

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

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 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:

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

Kirubel Aklilu (Gerrit)

unread,

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

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

6 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

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:

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

Reply all

Reply to author

Forward

0 new messages