[iOS] Update badges flow based on visibility [chromium/src : main]
Gauthier Ambard (Gerrit)
Gauthier Ambard voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ibrahim Kanouche
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 11 comments![]( "Open in Gerrit")
Thanks Gauthier, yeah this looks a lot better.
// Whether this badge mediator is visible or not.Let's add a statement on what this property does. If YES, becomes the delegate for `InfobarTabHelper`. If NO, removed as the delegate for `InfobarTabHelper`.
@property(nonatomic, assign) BOOL visible;Mediators aren't visible, maybe `active`?
- (void)setLocationBarVisible:(BOOL)visible;Same nit here `setLocationBarActive`
MainToolbarMediator* _mainToolbarMediator;nit: Comment above var
[self createLocationBarCoordinatorVisible:!isOmniboxInBottomPosition];Coordinators aren't really visible either, maybe `createLocationBarCoordinatorActive:(BOOL)active` wdyt?
[_topLocationBarCoordinator setLocationBarVisible:NO];Related to the comment in `badge_mediator`, what if we do an encompassing general function here:
`[_topLocationBarCoordinator setLocationBarActive:NO];` maybe updates from other components might be needed related to the omnibox position changing. wdyt?
/// Disconnects the observation.Nit: Used to disconnect any observations and clean up objects. (or something along those lines). This function may not be **only** used for disconnecting an observation.
/// The delegate to notify about changes.nit: to notify the mediator about changes.
/// Mediator for the main toolbar, observing preferences.nit:maybe more specific? observing preferences for omnibox positions?
PrefBackedBoolean* _bottomOmniboxEnabled;nit: comment above var.
Related details
- Ibrahim Kanouche
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. |
Ibrahim Kanouche (Gerrit)
Ibrahim Kanouche added 12 comments![]( "Open in Gerrit")
if (visible) {When visible transitions to NO, the InfobarBadgeTabHelper still holds this mediator as its delegate. This might be an issue if the badge tab helper later calls back into this mediator while it's "invisible".
Can we add an else branch where we call SetDelegate(nil) and clear badges?
if (visible) {nit: When becoming invisible, should the consumer also be notified to clear its displayed badges? Currently the consumer retains whatever badge state it had when this mediator was last visible. I am thinking if the UI for the hidden toolbar is still technically alive (just not the active one), it could display stale badges when it becomes visible again before `updateBadgesShownForWebState` runs.
InfobarBadgeTabHelper::FromWebState(_webState)->SetDelegate(self);nit: Inconsistent use of self.webState and _webState within the same block.
- (void)setLocationBarVisible:(BOOL)visible;Same nit here `setLocationBarActive`
nit: The name setLocationBarVisible (or setLocationBarActive) implies it controls the location bar's own visibility, but it actually controls the badge mediator's delegate registration. Maybe `setBadgeMediatorActive` or `setLocationBarBadgesActive`?
if (isIncognito && !IsChromeNextIaEnabled()) {Can we add a brief comment here explaining the replacement for incognito indicator as part of bling next (This would help future readers who lack context)?
#pragma mark - MainToolbarMediatorDelegateCan we move the delegate protocol implementations before the Private methods.
- (instancetype)init NS_UNAVAILABLE;should we also mark `new` as unavailable to prevent `[MainToolbarMediator new]`?
PrefBackedBoolean* _bottomOmniboxEnabled;nit: comment above var.
nit: `_bottomOmniboxEnabled` reads like a BOOL flag. How about `_bottomOmniboxPref`?
PrefBackedBoolean* _bottomOmniboxEnabled;`_bottomOmniboxEnabled` reads like a BOOL flag. How about `_bottomOmniboxPref`?
PrefBackedBoolean* _bottomOmniboxEnabled;`_bottomOmniboxEnabled` reads like a BOOL flag. How about `_bottomOmniboxPref`?
- (void)booleanDidChange:(id<ObservableBoolean>)observableBoolean {The coordinator's isOmniboxInBottomPosition guards on IsBottomOmniboxAvailable(), but this callback fires unconditionally when the pref changes. On devices where bottom omnibox isn't available (e.g.iPad), a pref toggle would still notify the delegate and trigger setLocationBarVisible: calls with incorrect values. We should either not observe the pref when !IsBottomOmniboxAvailable(), or add the guard here before calling the delegate.
Related details
- Gauthier Ambard
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 voted and added 19 comments![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Commit-Queue | +1 |
19 comments
Let's add a statement on what this property does. If YES, becomes the delegate for `InfobarTabHelper`. If NO, removed as the delegate for `InfobarTabHelper`.
Done
@property(nonatomic, assign) BOOL visible;Mediators aren't visible, maybe `active`?
Done
if (visible) {When visible transitions to NO, the InfobarBadgeTabHelper still holds this mediator as its delegate. This might be an issue if the badge tab helper later calls back into this mediator while it's "invisible".
Can we add an else branch where we call SetDelegate(nil) and clear badges?
We don't know if we are still the delegate or not when set to NO.
I have protected the callback to return early if not active.
if (visible) {nit: When becoming invisible, should the consumer also be notified to clear its displayed badges? Currently the consumer retains whatever badge state it had when this mediator was last visible. I am thinking if the UI for the hidden toolbar is still technically alive (just not the active one), it could display stale badges when it becomes visible again before `updateBadgesShownForWebState` runs.
I think it is fine?
In the sense that some other badges are not done through the badge tab helper, so we can't really fix all of them.
Also, if we display them before re-calling this method, it means the new badges won't be displayed.
InfobarBadgeTabHelper::FromWebState(_webState)->SetDelegate(self);nit: Inconsistent use of self.webState and _webState within the same block.
Done
Ibrahim KanoucheSame nit here `setLocationBarActive`
nit: The name setLocationBarVisible (or setLocationBarActive) implies it controls the location bar's own visibility, but it actually controls the badge mediator's delegate registration. Maybe `setBadgeMediatorActive` or `setLocationBarBadgesActive`?
This method is about the location bar. It is an implementation details that it is actually only forwarded to the badges. In the future it could be used for something else for example.
"active" is a good idea.
I am moving it to setLocationBarActive. I could also move it to setActive.
Can we add a brief comment here explaining the replacement for incognito indicator as part of bling next (This would help future readers who lack context)?
Moved to another CL.
MainToolbarMediator* _mainToolbarMediator;Gauthier Ambardnit: Comment above var
Done
[self createLocationBarCoordinatorVisible:!isOmniboxInBottomPosition];Coordinators aren't really visible either, maybe `createLocationBarCoordinatorActive:(BOOL)active` wdyt?
Done
Can we move the delegate protocol implementations before the Private methods.
Done
[_topLocationBarCoordinator setLocationBarVisible:NO];Related to the comment in `badge_mediator`, what if we do an encompassing general function here:
`[_topLocationBarCoordinator setLocationBarActive:NO];` maybe updates from other components might be needed related to the omnibox position changing. wdyt?
I am not sure to understand this comment. Could you clarify?
Nit: Used to disconnect any observations and clean up objects. (or something along those lines). This function may not be **only** used for disconnecting an observation.
Done
should we also mark `new` as unavailable to prevent `[MainToolbarMediator new]`?
We never do.
/// The delegate to notify about changes.Gauthier Ambardnit: to notify the mediator about changes.
I am not sure to understand. This is the mediator. Why would it notify itself?
Updated the comment.
/// Mediator for the main toolbar, observing preferences.nit:maybe more specific? observing preferences for omnibox positions?
Done
`_bottomOmniboxEnabled` reads like a BOOL flag. How about `_bottomOmniboxPref`?
Done
`_bottomOmniboxEnabled` reads like a BOOL flag. How about `_bottomOmniboxPref`?
Done
Ibrahim Kanouchenit: comment above var.
nit: `_bottomOmniboxEnabled` reads like a BOOL flag. How about `_bottomOmniboxPref`?
Done
- (void)booleanDidChange:(id<ObservableBoolean>)observableBoolean {The coordinator's isOmniboxInBottomPosition guards on IsBottomOmniboxAvailable(), but this callback fires unconditionally when the pref changes. On devices where bottom omnibox isn't available (e.g.iPad), a pref toggle would still notify the delegate and trigger setLocationBarVisible: calls with incorrect values. We should either not observe the pref when !IsBottomOmniboxAvailable(), or add the guard here before calling the delegate.
I don't fully agree with this. I think the delegate should be notified when the pref changes. However the value should take the availability into account.
Updated the getter but left this the same.
Related details
- Ibrahim Kanouche
- Joemer Ramos
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. |
Joemer Ramos (Gerrit)
Joemer Ramos added 2 comments![]( "Open in Gerrit")
[_topLocationBarCoordinator setLocationBarVisible:NO];Gauthier AmbardRelated to the comment in `badge_mediator`, what if we do an encompassing general function here:
`[_topLocationBarCoordinator setLocationBarActive:NO];` maybe updates from other components might be needed related to the omnibox position changing. wdyt?
I am not sure to understand this comment. Could you clarify?
my proposal is to change `setLocationBarVisible` to `setLocationBarActive` since we're activating a coordinator/mediator rather than making a UI element/view controller visible.
/// The delegate to notify about changes.Gauthier Ambardnit: to notify the mediator about changes.
I am not sure to understand. This is the mediator. Why would it notify itself?
Updated the comment.
- Gauthier Ambard
- Ibrahim Kanouche
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. |
Ibrahim Kanouche (Gerrit)
Ibrahim Kanouche added 2 comments![]( "Open in Gerrit")
if (visible) {Gauthier AmbardWhen visible transitions to NO, the InfobarBadgeTabHelper still holds this mediator as its delegate. This might be an issue if the badge tab helper later calls back into this mediator while it's "invisible".
Can we add an else branch where we call SetDelegate(nil) and clear badges?
We don't know if we are still the delegate or not when set to NO.
I have protected the callback to return early if not active.
Ack.
One question: in the setWebState: path, if visible == NO and the web state changes, the old web state's tab helper retains this mediator as delegate without cleanup. The early return guard prevents misbehavior, but is it guaranteed that the other (visible) mediator always sets itself as delegate on that same web state, effectively overwriting the stale reference? If so, this is fine. Just want to confirm there's no case where the old web state's helper holds a dangling delegate that never gets overwritten.
if (visible) {Gauthier Ambardnit: When becoming invisible, should the consumer also be notified to clear its displayed badges? Currently the consumer retains whatever badge state it had when this mediator was last visible. I am thinking if the UI for the hidden toolbar is still technically alive (just not the active one), it could display stale badges when it becomes visible again before `updateBadgesShownForWebState` runs.
I think it is fine?
In the sense that some other badges are not done through the badge tab helper, so we can't really fix all of them.
Also, if we display them before re-calling this method, it means the new badges won't be displayed.
Acknowledged
Related details
- Gauthier Ambard
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. |