[iOS] Add Passkey Creation Bottom Sheet class structure [chromium/src : main]
Alexis Hétu (Gerrit)
Alexis Hétu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Noémie St-Onge
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 26 comments![]( "Open in Gerrit")
I think we should take advantage of the fact that we're adding brand new files to follow the new folder structure guidance https://g3doc.corp.google.com/company/teams/chrome/ios/docs/engineering/internal_workflow/folder_structure.md#ioschromebrowser-structure
// `requestId` comes from the PasskeyTabHelper and identifies the passkey`requestID` as abbreviations are in all caps in Obj-C (same for the other instances below)
https://google.github.io/styleguide/objcguide.html#:~:text=This%20follows%20Apple%E2%80%99s%20standard%20of%20using%20all%20capitals%20within%20a%20name%20for%20acronyms%20such%20as%20URL%2C%20ID%2C%20TIFF%2C%20and%20EXIF.
@property(nonatomic, strong)
PasskeyCreationBottomSheetViewController* viewController;
@property(nonatomic, strong) PasskeyCreationBottomSheetMediator* mediator;we should prefer using ivars when possible + these should come with a comment
requestId:(const std::string&)requestId {`requestID`
ProfileIOS* profile = browser->GetProfile()->GetOriginalProfile();Why do we need the original profile here? We should explain it with a comment
ProfileIOS* profile = browser->GetProfile()->GetOriginalProfile();nit: we can use `self.profile` directly
self.mediator = [[PasskeyCreationBottomSheetMediator alloc]
initWithWebStateList:webStateList
requestId:requestId
accountForSaving:accountForSaving
presenter:self];
const GURL& URL = webStateList->GetActiveWebState()->GetLastCommittedURL();
self.viewController =
[[PasskeyCreationBottomSheetViewController alloc] initWithHandler:self
URL:URL];
self.viewController.actionHandler = self;
self.viewController.delegate = self.mediator;The mediator and vc (along with their dependencies) are typically initialized in the `-start` method rather than in the `-init` method. From what I understand from the [Coordinator fascicle](https://docs.google.com/document/d/1nyPbvbciPWXlj-oTC6CP0Z-jkERggd_9ZZjKQrLoBBU/edit?tab=t.0#heading=h.cgua8kfbjxyn), `-init` should be used for dependency injection, whereas `-start` should be used to activate the feature. I don't think we should be creating the mediator and VC in advance
- (void)viewDidDisappear;I would name this differently so that it doesn't get confused with Apple's `viewDidDisappear` function. We could also be more precise about the view that is implied, could be something like `passkeyCreationBottomSheetViewDidDisappear`
- (void)deferToRenderer;Suggestion as I feel like `deferToRenderer` could be harder to understand outside of the core passkey land ```suggestion
- (void)deferPasskeyCreationToRenderer;
- ```
requestId:(const std::string&)requestId`requestID`, same with argument name
// Email associated with account the passkey will get saved in.ß```suggestion
// Email associated with the account the passkey will get saved in.ß
```
// Email associated with account the passkey will get saved in.ß😅
@property(nonatomic, strong, readonly) NSString* accountForSaving;Should be an ivar if possible
requestId:(const std::string&)requestId`requestID`
if (_requestId.empty()) {Are we going to silently fail here? Should we CHECK instead? (Same question for `passkeyTabHelper`)
DCHECK_EQ(_webStateList, webStateList);Should this be `CHECK_EQ` to follow the new guidance?
DCHECK_EQ(webStateList, _webStateList);Same, should this be `CHECK_EQ` to follow the new guidance?
- (void)onWebStateChange {Missing a comment + suggestion ```suggestion
- (void)onWebStateChanged {
- ```
- (webauthn::PasskeyTabHelper*)tabHelper {Suggestion for clarity as we have multiple tab helpers ```suggestion
- (webauthn::PasskeyTabHelper*)passkeyTabHelper {
- ```
// Initialize with the handler for user actions and the URL of the current page.```suggestion
// Initializes the view controller with the `handler` for user actions and the `URL` of the current page.
```
@property(nonatomic, weak) id<PasskeyCreationBottomSheetHandler> handler;Switch to ivar if possible
// Set the properties read by the super when constructing the`base class`?
self.titleString =
l10n_util::GetNSString(IDS_IOS_PASSKEY_CREATION_BOTTOM_SHEET_TITLE);
self.titleTextStyle = UIFontTextStyleTitle2;
NSString* host = base::SysUTF8ToNSString(_URL.host());
NSString* username = [self.delegate username];
self.secondaryTitleString =
[NSString stringWithFormat:@"%@\n%@", username ?: @"", host];
NSString* subtitle =
l10n_util::GetNSString(IDS_IOS_PASSKEY_CREATION_BOTTOM_SHEET_SUBTITLE);
NSString* accountInfo =
l10n_util::GetNSString(IDS_IOS_PASSKEY_CREATION_BOTTOM_SHEET_ACCOUNT);
NSString* email = [self.delegate email];
self.subtitleString =
[NSString stringWithFormat:@"%@\n%@\n%@", subtitle, accountInfo, email];
self.subtitleTextStyle = UIFontTextStyleFootnote;optional: we could create helper functions for these setups to lighten the `viewDidLoad` method, which tends to get quite long. Not a strong opinion though since it is not that big right now
- (void)viewDidDisappear:(BOOL)animated {
[super viewDidDisappear:animated];
[self.handler viewDidDisappear];
}Forwarding a comment made by Gauthier on Angela's Test Project CL https://chromium-review.googlesource.com/c/chromium/src/+/7107319/comment/fbf500d5_04f46e06/
// Configures the title view of this ViewController.
- (UIView*)setUpTitleView {
NSString* title =
l10n_util::GetNSString(IDS_IOS_CREDENTIAL_BOTTOM_SHEET_TITLE);
UIView* titleView = password_manager::CreatePasswordManagerTitleView(title);
return titleView;
}I think this could be moved to the anonymous namespace since we're not accessing any members of `self`
Related details
- Alexis Hétu
- Tommy Martino
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. |
Alexis Hétu (Gerrit)
Alexis Hétu added 4 comments![]( "Open in Gerrit")
I think we should take advantage of the fact that we're adding brand new files to follow the new folder structure guidance https://g3doc.corp.google.com/company/teams/chrome/ios/docs/engineering/internal_workflow/folder_structure.md#ioschromebrowser-structure
Already working on this 😊
ProfileIOS* profile = browser->GetProfile()->GetOriginalProfile();Why do we need the original profile here? We should explain it with a comment
My understanding is that if we don't, it won't work in incognito.
if (_requestId.empty()) {Are we going to silently fail here? Should we CHECK instead? (Same question for `passkeyTabHelper`)
I could add a check in the init method and remove this if statement, PasskeyTabHelper should have already validated that request ID was valid.
- (void)viewDidDisappear:(BOOL)animated {
[super viewDidDisappear:animated];
[self.handler viewDidDisappear];
}Forwarding a comment made by Gauthier on Angela's Test Project CL https://chromium-review.googlesource.com/c/chromium/src/+/7107319/comment/fbf500d5_04f46e06/
I *think* this is ok since the view is modal, but I'll change it anyway. We'll probably want to update the other bottom sheets as well, once we've validated that the new way of doing this works for this bottom sheet.
Related details
- Noémie St-Onge
- Tommy Martino
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. |
Alexis Hétu (Gerrit)
Alexis Hétu voted and added 26 comments![]( "Open in Gerrit")
Votes added by Alexis Hétu
| Commit-Queue | +1 |
Alexis HétuI think we should take advantage of the fact that we're adding brand new files to follow the new folder structure guidance https://g3doc.corp.google.com/company/teams/chrome/ios/docs/engineering/internal_workflow/folder_structure.md#ioschromebrowser-structure
Already working on this 😊
Done
// `requestId` comes from the PasskeyTabHelper and identifies the passkey`requestID` as abbreviations are in all caps in Obj-C (same for the other instances below)
https://google.github.io/styleguide/objcguide.html#:~:text=This%20follows%20Apple%E2%80%99s%20standard%20of%20using%20all%20capitals%20within%20a%20name%20for%20acronyms%20such%20as%20URL%2C%20ID%2C%20TIFF%2C%20and%20EXIF.
Done
@property(nonatomic, strong)
PasskeyCreationBottomSheetViewController* viewController;
@property(nonatomic, strong) PasskeyCreationBottomSheetMediator* mediator;Alexis Hétuwe should prefer using ivars when possible + these should come with a comment
Done
requestId:(const std::string&)requestId {Alexis Hétu`requestID`
Done
ProfileIOS* profile = browser->GetProfile()->GetOriginalProfile();nit: we can use `self.profile` directly
Done
ProfileIOS* profile = browser->GetProfile()->GetOriginalProfile();Alexis HétuWhy do we need the original profile here? We should explain it with a comment
My understanding is that if we don't, it won't work in incognito.
Done
self.mediator = [[PasskeyCreationBottomSheetMediator alloc]
initWithWebStateList:webStateList
requestId:requestId
accountForSaving:accountForSaving
presenter:self];
const GURL& URL = webStateList->GetActiveWebState()->GetLastCommittedURL();
self.viewController =
[[PasskeyCreationBottomSheetViewController alloc] initWithHandler:self
URL:URL];
self.viewController.actionHandler = self;
self.viewController.delegate = self.mediator;The mediator and vc (along with their dependencies) are typically initialized in the `-start` method rather than in the `-init` method. From what I understand from the [Coordinator fascicle](https://docs.google.com/document/d/1nyPbvbciPWXlj-oTC6CP0Z-jkERggd_9ZZjKQrLoBBU/edit?tab=t.0#heading=h.cgua8kfbjxyn), `-init` should be used for dependency injection, whereas `-start` should be used to activate the feature. I don't think we should be creating the mediator and VC in advance
Done
I would name this differently so that it doesn't get confused with Apple's `viewDidDisappear` function. We could also be more precise about the view that is implied, could be something like `passkeyCreationBottomSheetViewDidDisappear`
Done
Suggestion as I feel like `deferToRenderer` could be harder to understand outside of the core passkey land ```suggestion
- (void)deferPasskeyCreationToRenderer;
- ```
Done
`requestID`, same with argument name
Done
// Email associated with account the passkey will get saved in.ßAlexis Hétu😅
Done
// Email associated with account the passkey will get saved in.ß```suggestion
// Email associated with the account the passkey will get saved in.ß
```
Done
@property(nonatomic, strong, readonly) NSString* accountForSaving;Should be an ivar if possible
Done
requestId:(const std::string&)requestIdAlexis Hétu`requestID`
Done
Alexis HétuAre we going to silently fail here? Should we CHECK instead? (Same question for `passkeyTabHelper`)
I could add a check in the init method and remove this if statement, PasskeyTabHelper should have already validated that request ID was valid.
For PasskeyTabHelper, if might be gone if we navigated to another page, so it might be nil, but we'd want to do nothing in that case, so I think that's ok to leave as is.
Should this be `CHECK_EQ` to follow the new guidance?
Done
Same, should this be `CHECK_EQ` to follow the new guidance?
Done
Missing a comment + suggestion ```suggestion
- (void)onWebStateChanged {
- ```
Done
Suggestion for clarity as we have multiple tab helpers ```suggestion
- (webauthn::PasskeyTabHelper*)passkeyTabHelper {
- ```
Done
// Initialize with the handler for user actions and the URL of the current page.```suggestion
// Initializes the view controller with the `handler` for user actions and the `URL` of the current page.
```
Done
@property(nonatomic, weak) id<PasskeyCreationBottomSheetHandler> handler;Switch to ivar if possible
Done
// Set the properties read by the super when constructing theAlexis Hétu`base class`?
Done
self.titleString =
l10n_util::GetNSString(IDS_IOS_PASSKEY_CREATION_BOTTOM_SHEET_TITLE);
self.titleTextStyle = UIFontTextStyleTitle2;
NSString* host = base::SysUTF8ToNSString(_URL.host());
NSString* username = [self.delegate username];
self.secondaryTitleString =
[NSString stringWithFormat:@"%@\n%@", username ?: @"", host];
NSString* subtitle =
l10n_util::GetNSString(IDS_IOS_PASSKEY_CREATION_BOTTOM_SHEET_SUBTITLE);
NSString* accountInfo =
l10n_util::GetNSString(IDS_IOS_PASSKEY_CREATION_BOTTOM_SHEET_ACCOUNT);
NSString* email = [self.delegate email];
self.subtitleString =
[NSString stringWithFormat:@"%@\n%@\n%@", subtitle, accountInfo, email];
self.subtitleTextStyle = UIFontTextStyleFootnote;optional: we could create helper functions for these setups to lighten the `viewDidLoad` method, which tends to get quite long. Not a strong opinion though since it is not that big right now
Done
- (void)viewDidDisappear:(BOOL)animated {
[super viewDidDisappear:animated];
[self.handler viewDidDisappear];
}Alexis HétuForwarding a comment made by Gauthier on Angela's Test Project CL https://chromium-review.googlesource.com/c/chromium/src/+/7107319/comment/fbf500d5_04f46e06/
I *think* this is ok since the view is modal, but I'll change it anyway. We'll probably want to update the other bottom sheets as well, once we've validated that the new way of doing this works for this bottom sheet.
Replaced with `didMoveToParentViewController`.
// Configures the title view of this ViewController.
- (UIView*)setUpTitleView {
NSString* title =
l10n_util::GetNSString(IDS_IOS_CREDENTIAL_BOTTOM_SHEET_TITLE);
UIView* titleView = password_manager::CreatePasswordManagerTitleView(title);
return titleView;
}I think this could be moved to the anonymous namespace since we're not accessing any members of `self`
Done
Related details
- Noémie St-Onge
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge voted and added 8 comments![]( "Open in Gerrit")
Votes added by Noémie St-Onge
| Code-Review | +1 |
8 comments
LGTM with nits! Request for next time: please consider breaking such CLs into smaller pieces. This CL is quite large, which makes it harder to reason about all at once
// `viewController` is the VC used to present the bottom sheet.```suggestion
// `viewController` is the view controller used to present the bottom sheet.
```
PasskeyCreationBottomSheetViewController* _viewController;
PasskeyCreationBottomSheetMediator* _mediator;
std::string _requestID;Missing comments
// Use GetOriginalProfile so that it uses the signed in profile even when
// incognito mode.```suggestion
// Use GetOriginalProfile so that it uses the signed in profile even when in
// incognito mode.
```
// Presenter that controls the presentation of the bottom sheet.
@property(nonatomic, weak) id<PasskeyCreationBottomSheetPresenter> presenter;Could this be an ivar?
UIView* setUpTitleView() {Should start with a capital `S`
```suggestion
UIView* SetUpTitleView() {
```
UIView* titleView = password_manager::CreatePasswordManagerTitleView(title);`title_view`
Related details
- Alexis Hétu
- Tommy Martino
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. |
Alexis Hétu (Gerrit)
Alexis Hétu voted and added 8 comments![]( "Open in Gerrit")
Votes added by Alexis Hétu
| Commit-Queue | +1 |
LGTM with nits! Request for next time: please consider breaking such CLs into smaller pieces. This CL is quite large, which makes it harder to reason about all at once
Ah yes, sorry about that, I should've split it once I split the ui and coordinator portions, but I didn't think about it.
// `viewController` is the VC used to present the bottom sheet.```suggestion
// `viewController` is the view controller used to present the bottom sheet.
```
Done
PasskeyCreationBottomSheetViewController* _viewController;
PasskeyCreationBottomSheetMediator* _mediator;
std::string _requestID;Alexis HétuMissing comments
Done
// Use GetOriginalProfile so that it uses the signed in profile even when
// incognito mode.```suggestion
// Use GetOriginalProfile so that it uses the signed in profile even when in
// incognito mode.
```
Done
// Presenter that controls the presentation of the bottom sheet.
@property(nonatomic, weak) id<PasskeyCreationBottomSheetPresenter> presenter;Could this be an ivar?
Done
- (void)onWebStateChanged {Alexis HétuMissing comment
Done
Should start with a capital `S`
```suggestion
UIView* SetUpTitleView() {
```
Done
UIView* titleView = password_manager::CreatePasswordManagerTitleView(title);Alexis Hétu`title_view`
Done
Related details
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino added 13 comments![]( "Open in Gerrit")
requestID:(const std::string&)requestID;There's no real point in taking this by constref if the first thing we're going to do is copy it into a member variable anyway. Let's change this parameter to pass-by-value (and then `std::move` in the initializer).
That way (a) the lifetime is unambiguous (the caller knows they don't have to persist `requestID` throughout the lifetime of this object) and (b) if the caller doesn't need `requestID` anymore, they can `std::move` into it and save a copy.
: ChromeCoordinator <PasskeyCreationBottomSheetPresenter>Nit: the fact that this is a `PasskeyCreationBottomSheetPresenter` (MediatorDelegate) does not need to be part of the public interface, as it relates only to the instantiation of the mediator (which happens in the impl). This can be declared in the .mm file instead.
std::string _requestID;There isn't really a reason for the coordinator to know about the request ID (which is really model-layer information), except so that it can pass it on to the mediator. Let's make this an `std::optional<std::string> _pendingRequestID`, and move + reset when we create the mediator.
// incognito mode.Whenever we do this, I like for the comment to document *why* it's OK to use the original profile (so, in this case, the user is later asked to make an explicit choice about saving this data to their account).
std::unique_ptr<ActiveWebStateObservationForwarder> _forwarder;I don't think we actually need to forward from the ActiveWebState; it should be sufficient to observe the WebState that's active at initialization.
This sheet is presented over a single WebState/tab that shouldn't change (as it's modal). If the active tab *does* somehow change, then we'll be notified via `didChangeWebStateList` and will close anyway.
// Bridge and forwarder for observing WebState events. The forwarder is a
// scoped observation, so the bridge will automatically be removed from the
// relevant observer list.
std::unique_ptr<web::WebStateObserverBridge> _webStateObserver;
std::unique_ptr<ActiveWebStateObservationForwarder> _forwarder;
// Bridge for observing WebStateList events.
std::unique_ptr<WebStateListObserverBridge> _webStateListObserver;
std::unique_ptr<
base::ScopedObservation<WebStateList, WebStateListObserverBridge>>
_webStateListObservation;For objects that don't ever get moved, avoid using unique_ptr. Ideally we would just make the object an ivar of this class directly, but since I think none of these are/can be default-constructed, we can use std::optional to handle the uninitialized state.
@protocol PasskeyCreationBottomSheetPresenter <NSObject>I believe the preferred term for the protocol that a mediator uses to talk to a coordinator is now `FooMediatorDelegate` rather than `FooPresenter`.
Could we also pass URL via the same mechanism as username/email?
@protocol PasskeyCreationBottomSheetDelegateGenerally, the protocol whereby a VC talks to a Mediator would be called a Mutator.
However, in this case, I think the model is somewhat backwards – typically in the CMVC architecture, we push data to a receiver (ref. the diagram in https://docs.google.com/document/d/1YjLSWwUgBy4OPJZtKUlypu2VseLufrodCxL-wU1kdEM/edit?tab=t.0#heading=h.qc4f3ngdx65b) rather than pulling it from a delegate.
IOW, we would want a *consumer* protocol that lets the mediator provide the username/email to the VC, and then any mutator protocol (if necessary) would be used to trigger model updates (e.g., "the user said OK, so please create this passkey").
@property(nonatomic, strong) id<PasskeyCreationBottomSheetDelegate> delegate;Does this need to be `strong`?
URL:(const GURL&)URL;Same deal as with the string in the coordinator/mediator -- let's take this by (possibly-moved) value since we're ultimately copying it anyway.
UIView* title_view = password_manager::CreatePasswordManagerTitleView(title);Nit, not for this CL: probably `create_password_manager_title_view` should be moved into icb/passwords/ui, since we now use it in a lot of places beyond settings.
Please do not add new stuff to `ui_bundled` directories. Instead, let's create an `icb/passwords/passkey_creation/` directory (with its own `coordinator`, `model`, `ui`, etc subfolders as needed).
Related details
- Alexis Hétu
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Noémie St-Onge
| Code-Review | +1 |
1 comment
// upon calling -viewDidDisappear.forgot to add this comment earlier: I think this is obsolete now
Alexis Hétu (Gerrit)
Alexis Hétu voted and added 13 comments![]( "Open in Gerrit")
Votes added by Alexis Hétu
| Commit-Queue | +1 |
13 comments
requestID:(const std::string&)requestID;There's no real point in taking this by constref if the first thing we're going to do is copy it into a member variable anyway. Let's change this parameter to pass-by-value (and then `std::move` in the initializer).
That way (a) the lifetime is unambiguous (the caller knows they don't have to persist `requestID` throughout the lifetime of this object) and (b) if the caller doesn't need `requestID` anymore, they can `std::move` into it and save a copy.
Done
: ChromeCoordinator <PasskeyCreationBottomSheetPresenter>Nit: the fact that this is a `PasskeyCreationBottomSheetPresenter` (MediatorDelegate) does not need to be part of the public interface, as it relates only to the instantiation of the mediator (which happens in the impl). This can be declared in the .mm file instead.
Done
std::string _requestID;There isn't really a reason for the coordinator to know about the request ID (which is really model-layer information), except so that it can pass it on to the mediator. Let's make this an `std::optional<std::string> _pendingRequestID`, and move + reset when we create the mediator.
Done
forgot to add this comment earlier: I think this is obsolete now
Done
// incognito mode.Whenever we do this, I like for the comment to document *why* it's OK to use the original profile (so, in this case, the user is later asked to make an explicit choice about saving this data to their account).
Done
std::unique_ptr<ActiveWebStateObservationForwarder> _forwarder;I don't think we actually need to forward from the ActiveWebState; it should be sufficient to observe the WebState that's active at initialization.
This sheet is presented over a single WebState/tab that shouldn't change (as it's modal). If the active tab *does* somehow change, then we'll be notified via `didChangeWebStateList` and will close anyway.
Done
// Bridge and forwarder for observing WebState events. The forwarder is a
// scoped observation, so the bridge will automatically be removed from the
// relevant observer list.
std::unique_ptr<web::WebStateObserverBridge> _webStateObserver;
std::unique_ptr<ActiveWebStateObservationForwarder> _forwarder;
// Bridge for observing WebStateList events.
std::unique_ptr<WebStateListObserverBridge> _webStateListObserver;
std::unique_ptr<
base::ScopedObservation<WebStateList, WebStateListObserverBridge>>
_webStateListObservation;For objects that don't ever get moved, avoid using unique_ptr. Ideally we would just make the object an ivar of this class directly, but since I think none of these are/can be default-constructed, we can use std::optional to handle the uninitialized state.
Done
@protocol PasskeyCreationBottomSheetPresenter <NSObject>I believe the preferred term for the protocol that a mediator uses to talk to a coordinator is now `FooMediatorDelegate` rather than `FooPresenter`.
Done
Could we also pass URL via the same mechanism as username/email?
Done
@protocol PasskeyCreationBottomSheetDelegateGenerally, the protocol whereby a VC talks to a Mediator would be called a Mutator.
However, in this case, I think the model is somewhat backwards – typically in the CMVC architecture, we push data to a receiver (ref. the diagram in https://docs.google.com/document/d/1YjLSWwUgBy4OPJZtKUlypu2VseLufrodCxL-wU1kdEM/edit?tab=t.0#heading=h.qc4f3ngdx65b) rather than pulling it from a delegate.
IOW, we would want a *consumer* protocol that lets the mediator provide the username/email to the VC, and then any mutator protocol (if necessary) would be used to trigger model updates (e.g., "the user said OK, so please create this passkey").
Done
@property(nonatomic, strong) id<PasskeyCreationBottomSheetDelegate> delegate;Does this need to be `strong`?
Removed.
URL:(const GURL&)URL;Same deal as with the string in the coordinator/mediator -- let's take this by (possibly-moved) value since we're ultimately copying it anyway.
Done
UIView* title_view = password_manager::CreatePasswordManagerTitleView(title);Nit, not for this CL: probably `create_password_manager_title_view` should be moved into icb/passwords/ui, since we now use it in a lot of places beyond settings.
- Noémie St-Onge
- Tommy Martino
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. |
Alexis Hétu (Gerrit)
Alexis Hétu added 1 comment![]( "Open in Gerrit")
Please do not add new stuff to `ui_bundled` directories. Instead, let's create an `icb/passwords/passkey_creation/` directory (with its own `coordinator`, `model`, `ui`, etc subfolders as needed).
Done
Related details
- Noémie St-Onge
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted and added 7 comments![]( "Open in Gerrit")
Votes added by Tommy Martino
| Code-Review | +1 |
7 comments
const GURL& URL = webStateList->GetActiveWebState()->GetLastCommittedURL();Nit: the mediator receives webStateList, so no need to get and pass this URL separately
- (void)setUsername:(NSString*)username email:(NSString*)email url:(GURL)url;URL
}Should this trigger a refresh of the UI elements, so that the new values are reflected in `secondaryTitle` and `subtitle` if set after `viewDidLoad`?
Please do not add new stuff to `ui_bundled` directories. Instead, let's create an `icb/passwords/passkey_creation/` directory (with its own `coordinator`, `model`, `ui`, etc subfolders as needed).
- Alexis Hétu
- Noémie St-Onge
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. |