[ios] Split TaskRequest responsibilities in multiple classes [chromium/src : main]
0 views
Skip to first unread message
Federica Germinario (Gerrit)
Feb 23, 2026, 8:15:29 AM (yesterday) Feb 23
to Sylvain Defresne, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Sylvain Defresne
Federica Germinario added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Sylvain Defresne
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Feb 23, 2026, 11:28:01 AM (yesterday) Feb 23
to Federica Germinario, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Federica Germinario
Sylvain Defresne added 5 comments![]( "Open in Gerrit")
File ios/chrome/app/task_request.h
Line 70, Patchset 5 (Latest):
// Protected methods for subclasses.Sylvain Defresne . unresolved
I think we can move this to `task_request_private.h`, which we allow to include from sub-classes and from `task_request.mm` but not from the rest of the codebase.
```
@interface TaskRequest ()
- (instancetype)initWithSceneState:(SceneState*)sceneState
isColdStart:(BOOL)isColdStart;
- (SceneState*)sceneStateFromSessionID;
@end
```
Line 38, Patchset 5 (Latest):
@protected
std::string _sceneSessionID;
__weak SceneState* _sceneState;
BOOL _isColdStart;
}Sylvain Defresne . unresolved
I don't think this is necessary.
Looking at the implementation, this is never used from sub-classes. The sub-classes uses the `isColdStart` property, and the `sceneStateFromSessionID` methods instead of accessing those ivars directly.
Thus you should be able to move this back into the implementation files.
File ios/chrome/app/task_request.mm
Line 102, Patchset 5 (Latest):
+ (instancetype)taskForTestingWithSceneID:(std::string_view)sceneIDSylvain Defresne . unresolved
nit: move with the other factories (i.e. before the initializers)
Line 128, Patchset 5 (Latest):
return nil;Sylvain Defresne . unresolved
nit: `NOTREACHED()` since all sub-classes `CHECK(...)` that the pointer returned is not nil
File ios/chrome/app/task_request_shortcut_item.mm
Line 27, Patchset 5 (Latest):
_shortcutHandler = [handler copy];Sylvain Defresne . unresolved
nit: `-copy` is not required since we use ARC.
Related details
Attention is currently required from:
- Federica Germinario
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. |
Federica Germinario (Gerrit)
8:17 AM (8 hours ago) 8:17 AM
to Sylvain Defresne, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Sylvain Defresne
Federica Germinario added 5 comments![]( "Open in Gerrit")
File ios/chrome/app/task_request.h
I think we can move this to `task_request_private.h`, which we allow to include from sub-classes and from `task_request.mm` but not from the rest of the codebase.
```
@interface TaskRequest ()- (instancetype)initWithSceneState:(SceneState*)sceneState
isColdStart:(BOOL)isColdStart;
- (SceneState*)sceneStateFromSessionID;
@end
```
Federica Germinario
Done
Line 38, Patchset 5:
@protected
std::string _sceneSessionID;
__weak SceneState* _sceneState;
BOOL _isColdStart;
}Sylvain Defresne . resolved
I don't think this is necessary.
Looking at the implementation, this is never used from sub-classes. The sub-classes uses the `isColdStart` property, and the `sceneStateFromSessionID` methods instead of accessing those ivars directly.
Thus you should be able to move this back into the implementation files.
Federica Germinario
Done
File ios/chrome/app/task_request.mm
Line 102, Patchset 5:
+ (instancetype)taskForTestingWithSceneID:(std::string_view)sceneIDSylvain Defresne . resolved
nit: move with the other factories (i.e. before the initializers)
Federica Germinario
Done
nit: `NOTREACHED()` since all sub-classes `CHECK(...)` that the pointer returned is not nil
Federica Germinario
Done
File ios/chrome/app/task_request_shortcut_item.mm
nit: `-copy` is not required since we use ARC.
Attention is currently required from:
- Sylvain Defresne
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
11:49 AM (5 hours ago) 11:49 AM
to Federica Germinario, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Federica Germinario
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Federica Germinario
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Federica Germinario (Gerrit)
11:50 AM (5 hours ago) 11:50 AM
to Sylvain Defresne, Chromium LUCI CQ, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Federica Germinario voted and added 1 comment![]( "Open in Gerrit")
Votes added by Federica Germinario
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
12:56 PM (4 hours ago) 12:56 PM
to Federica Germinario, Sylvain Defresne, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[ios] Split TaskRequest responsibilities in multiple classes
Add:
- TaskRequestForShortcutItem
- TaskRequestForURLContext
- TaskRequestForUserActivity
classes to handle specific intent types, to avoid having all logic in
one single class.
Bug: 462018636
Change-Id: I216fde6a31444cfcee40ce91e48f732f1eb64440
Reviewed-by: Sylvain Defresne <sdef...@chromium.org>
Commit-Queue: Federica Germinario <fede...@google.com>
Cr-Commit-Position: refs/heads/main@{#1589505}
Files:
- M ios/chrome/app/BUILD.gn
- M ios/chrome/app/task_request.h
- M ios/chrome/app/task_request.mm
- A ios/chrome/app/task_request_private.h
- A ios/chrome/app/task_request_shortcut_item.h
- A ios/chrome/app/task_request_shortcut_item.mm
- A ios/chrome/app/task_request_url_context.h
- A ios/chrome/app/task_request_url_context.mm
- A ios/chrome/app/task_request_user_activity.h
- A ios/chrome/app/task_request_user_activity.mm
- M ios/chrome/browser/shared/coordinator/scene/scene_delegate.mm
Change size: L
Delta: 11 files changed, 386 insertions(+), 220 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Sylvain Defresne
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages