[iOS] Use delegate pattern for ReminderNotificationsCoordinator lifecycle [chromium/src : main]
Karis Li (Gerrit)
Karis Li voted and added 1 comment![]( "Open in Gerrit")
Votes added by Karis Li
| Commit-Queue | +1 |
1 comment
Related details
- Gauthier Ambard
- Sylvain Defresne
- Éric Noyau
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 2 comments![]( "Open in Gerrit")
[iOS] Use delegate pattern for ReminderNotificationsCoordinator lifecycleSuper interesting question!
Adding @bwwil...@google.com as author of the todo
[handler dismissSetTabReminderUI];How did that work when it was presented from the bookmarks?
Related details
- Benjamin Williams
- Karis Li
- Sylvain Defresne
- Éric Noyau
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. |
Karis Li (Gerrit)
Karis Li added 1 comment![]( "Open in Gerrit")
[handler dismissSetTabReminderUI];How did that work when it was presented from the bookmarks?
Previously this didn't work correctly — dismissSetTabReminderUI was dispatched through CommandDispatcher and handled by BrowserCoordinator, but when the reminder was presented from BookmarksCoordinator, BrowserCoordinator's _reminderNotificationsCoordinator was nil, so the dismiss was a no-op and the UI got stuck. This CL fixes that bug by using the delegate pattern so each presenter handles its own coordinator lifecycle.
Before: https://drive.google.com/file/d/1NcvzF12ATbCkPMe9Bf5CfIg0Z4t6sLXR/view?usp=sharing
After: https://drive.google.com/file/d/16knhPyKE8QhNExdFXgT4UMgiiMBzJ76h/view?usp=sharing
Related details
- Benjamin Williams
- Gauthier Ambard
- Sylvain Defresne
- Éric Noyau
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Code-Review | +1 |
1 comment
[handler dismissSetTabReminderUI];Karis LiHow did that work when it was presented from the bookmarks?
Previously this didn't work correctly — dismissSetTabReminderUI was dispatched through CommandDispatcher and handled by BrowserCoordinator, but when the reminder was presented from BookmarksCoordinator, BrowserCoordinator's _reminderNotificationsCoordinator was nil, so the dismiss was a no-op and the UI got stuck. This CL fixes that bug by using the delegate pattern so each presenter handles its own coordinator lifecycle.
Before: https://drive.google.com/file/d/1NcvzF12ATbCkPMe9Bf5CfIg0Z4t6sLXR/view?usp=sharing
After: https://drive.google.com/file/d/16knhPyKE8QhNExdFXgT4UMgiiMBzJ76h/view?usp=sharing
- Benjamin Williams
- Karis Li
- Sylvain Defresne
- Éric Noyau
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. |
Éric Noyau (Gerrit)
Éric Noyau added 6 comments![]( "Open in Gerrit")
[_reminderNotificationsCoordinator stop];Should this be `[coordinator stop]`? Or at a minimum a `CHECK` that `coordinator` and `_reminderNotificationsCoordinator` are the same object?
Is it possible for two ReminderNotificationsCoordinator to be active at the same time? The place where the `_reminderNotificationsCoordinator` is set doesn't check if it is nil first, and doesn't try to remove itself as a delegate…
[_reminderNotificationsCoordinator stop];So the first thing that happens in a method named `-reminderNotificationsCoordinatorDidStop:` is a call to `-stop` on the aforementioned coordinator? How can this be `DidStop` if it has not stopped before? According to its name, the call to `-stop` must be done by the coordinator before calling its delegate, otherwise this makes no sense.
[self stopReminderNotificationsCoordinator];Same here, a `DidStop` method is invoked for something that has not been stopped yet.
@protocol ReminderNotificationsCoordinatorDelegate;Forward declarations of types are avoided, just import the relevant headers where needed. Our internal ObjectiveC style guide explicitly requires it, but I just noticed this hasn't made it into the public facing style guide yet. I've asked for it to be refreshed.
[self.delegate reminderNotificationsCoordinatorDidStop:self];This should probably be:
```
{
[self stop];
[self.delegate reminderNotificationsCoordinatorDidStop:self];
}
```
@class ReminderNotificationsCoordinator;Delegate protocols are usually declared in the same header as the delegating class, as it is part of the API of the class. This removes the need for a forward declaration (There is internal guidance for this: go/objc-delegate#heading=h.zedb0g579btl).
Related details
- Benjamin Williams
- Karis Li
- Sylvain Defresne
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. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 1 comment![]( "Open in Gerrit")
[self.delegate reminderNotificationsCoordinatorDidStop:self];This should probably be:
```
{
[self stop];
[self.delegate reminderNotificationsCoordinatorDidStop:self];
}
```
Coordinators are not supposed to call stop on themselves. It is the parent that is calling -stop.
Benjamin Williams (Gerrit)
Benjamin Williams added 2 comments![]( "Open in Gerrit")
[iOS] Use delegate pattern for ReminderNotificationsCoordinator lifecycleSuper interesting question!
Adding @bwwil...@google.com as author of the todo
Thanks for flagging! For add'l context, this feature is still in development (not code complete yet.)
[handler dismissSetTabReminderUI];How did that work when it was presented from the bookmarks?
This project was never code complete, so not entirely sure the current state. I'd say it's 80-85% complete.
Related details
- Karis Li
- Sylvain Defresne
Sylvain Defresne (Gerrit)
Sylvain Defresne added 2 comments![]( "Open in Gerrit")
I agree with other points by Éric (besides the forward-declaration).
@protocol ReminderNotificationsCoordinatorDelegate;Forward declarations of types are avoided, just import the relevant headers where needed. Our internal ObjectiveC style guide explicitly requires it, but I just noticed this hasn't made it into the public facing style guide yet. I've asked for it to be refreshed.
The Chromium style guide explicitly states that forward-declarations are preferred to includes/imports because forward-declarations can significantly reduce compilation times.
The reason is that not everyone is able to use distributed build (and thus forward-declaration is really helpful for them), and even when using distributed build we have to upload all the included files (in case there are local modifications) which is less performant than just forward-declaration.
This is true for both C++ and Objective-C++ file.
Related details
- Karis Li
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. |
Karis Li (Gerrit)
Karis Li added 6 comments![]( "Open in Gerrit")
Hi reviewers, could you please help review again? Thank you very much.
[iOS] Use delegate pattern for ReminderNotificationsCoordinator lifecycleBenjamin WilliamsSuper interesting question!
Adding @bwwil...@google.com as author of the todo
Thanks for flagging! For add'l context, this feature is still in development (not code complete yet.)
Marked as resolved.
Should this be `[coordinator stop]`? Or at a minimum a `CHECK` that `coordinator` and `_reminderNotificationsCoordinator` are the same object?
Is it possible for two ReminderNotificationsCoordinator to be active at the same time? The place where the `_reminderNotificationsCoordinator` is set doesn't check if it is nil first, and doesn't try to remove itself as a delegate…
Marked as resolved.
Sylvain DefresneForward declarations of types are avoided, just import the relevant headers where needed. Our internal ObjectiveC style guide explicitly requires it, but I just noticed this hasn't made it into the public facing style guide yet. I've asked for it to be refreshed.
The Chromium style guide explicitly states that forward-declarations are preferred to includes/imports because forward-declarations can significantly reduce compilation times.
The reason is that not everyone is able to use distributed build (and thus forward-declaration is really helpful for them), and even when using distributed build we have to upload all the included files (in case there are local modifications) which is less performant than just forward-declaration.
This is true for both C++ and Objective-C++ file.
Marked as resolved.
[handler dismissSetTabReminderUI];Benjamin WilliamsHow did that work when it was presented from the bookmarks?
This project was never code complete, so not entirely sure the current state. I'd say it's 80-85% complete.
Acknowledged
Delegate protocols are usually declared in the same header as the delegating class, as it is part of the API of the class. This removes the need for a forward declaration (There is internal guidance for this: go/objc-delegate#heading=h.zedb0g579btl).
- Benjamin Williams
- Gauthier Ambard
- Sylvain Defresne
- Éric Noyau
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. |
Karis Li (Gerrit)
Karis Li added 3 comments![]( "Open in Gerrit")
[_reminderNotificationsCoordinator stop];Karis LiSo the first thing that happens in a method named `-reminderNotificationsCoordinatorDidStop:` is a call to `-stop` on the aforementioned coordinator? How can this be `DidStop` if it has not stopped before? According to its name, the call to `-stop` must be done by the coordinator before calling its delegate, otherwise this makes no sense.
Acknowledged
Same here, a `DidStop` method is invoked for something that has not been stopped yet.
Marked as resolved.
[self.delegate reminderNotificationsCoordinatorDidStop:self];Gauthier AmbardThis should probably be:
```
{
[self stop];
[self.delegate reminderNotificationsCoordinatorDidStop:self];
}
```
Coordinators are not supposed to call stop on themselves. It is the parent that is calling -stop.
Thanks,done
Related details
- Benjamin Williams
- Gauthier Ambard
- Sylvain Defresne
- Éric Noyau
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Éric Noyau (Gerrit)
Éric Noyau added 1 comment![]( "Open in Gerrit")
@protocol ReminderNotificationsCoordinatorDelegate;Sylvain DefresneForward declarations of types are avoided, just import the relevant headers where needed. Our internal ObjectiveC style guide explicitly requires it, but I just noticed this hasn't made it into the public facing style guide yet. I've asked for it to be refreshed.
The Chromium style guide explicitly states that forward-declarations are preferred to includes/imports because forward-declarations can significantly reduce compilation times.
The reason is that not everyone is able to use distributed build (and thus forward-declaration is really helpful for them), and even when using distributed build we have to upload all the included files (in case there are local modifications) which is less performant than just forward-declaration.
This is true for both C++ and Objective-C++ file.
Ah, sorry, I spend too much of my review time on non Chromium code.
Related details
- Benjamin Williams
- Gauthier Ambard
- Karis Li
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted Code-Review+1![]( "Open in Gerrit")
- Benjamin Williams
- Karis Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |