[BWG] Implement multi active badge state. [chromium/src : main]
Ibrahim Kanouche (Gerrit)
Ibrahim Kanouche voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- 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 6 comments![]( "Open in Gerrit")
#import "ios/chrome/browser/badges/ui_bundled/badge_type.h"Don't we need this?
if (IsProactiveSuggestionsFrameworkEnabled() && self.webState) {Can we have a TODO over this to migrate to LocationBarBadge? Maybe under this bug: b/458307626
if (self.webState->GetStateForPermission(web::PermissionCamera) ==
web::PermissionStateAllowed) {This code is triggered multiple times when `updateBadgesShownForWebState` is called. Did you encounter any issues where we were adding more than one camera badge or more than one microphone badge at all? Just making sure there's no issues.
I ran into a UI bug where if I refreshed after enabling microphone and camera, there would be 2 microphones, or an incorrect icon.
[self.stackView addArrangedSubview:badgeButton];Change to `[self addBadgeToStackView:badgeButton];` so that the view animates in just like the first displayed badge (unless you don't want this behavior)
readerModeChipShouldBeVisibleFinal = _readerModeChipShouldBeVisible;Why do we do this?
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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Ibrahim Kanouche
| Commit-Queue | +1 |
6 comments
#import "ios/chrome/browser/badges/ui_bundled/badge_type.h"Don't we need this?
Done
if (IsProactiveSuggestionsFrameworkEnabled() && self.webState) {Can we have a TODO over this to migrate to LocationBarBadge? Maybe under this bug: b/458307626
Done
if (self.webState->GetStateForPermission(web::PermissionCamera) ==
web::PermissionStateAllowed) {This code is triggered multiple times when `updateBadgesShownForWebState` is called. Did you encounter any issues where we were adding more than one camera badge or more than one microphone badge at all? Just making sure there's no issues.
I ran into a UI bug where if I refreshed after enabling microphone and camera, there would be 2 microphones, or an incorrect icon.
I am not sure how you get to a state where you add more than one camera badge, it should either be one camera badge or zero camera badges (unless you are referring to adding a camera badge and a microphone badge).
As for refreshing, this should automatically revoke the camera or microphone permission and all the badges should disappear. I am not able to reproduce the bug you are describing. Can you share more info and/or reproduction steps?
The only issue I have been running into is adding a second permission badge after the first infobar is dismissed. The issue is that both camera and microphone permissions share the same underlying infobar type (InfobarType::kInfobarTypePermissions), but we are trying to create separate badges for each permission. I updated updateBadgesShownForWebState to fix this issue by making separate calls to updateDisplayedBadge for each badge.
Change to `[self addBadgeToStackView:badgeButton];` so that the view animates in just like the first displayed badge (unless you don't want this behavior)
Done
readerModeChipShouldBeVisibleFinal = _readerModeChipShouldBeVisible;Why do we do this?
This line directly assigns the reader mode chip's desired visibility state to the final visibility variable without applying any additional conditions. Because when ProactiveSuggestionsFramework is enabled, we simplify Reader Mode visibility by removing the legacy conflict logic that prevented it from showing during contextual panel animations.
Related details
- 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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Code-Review | +1 |
if (self.webState->GetStateForPermission(web::PermissionCamera) ==
web::PermissionStateAllowed) {Ibrahim KanoucheThis code is triggered multiple times when `updateBadgesShownForWebState` is called. Did you encounter any issues where we were adding more than one camera badge or more than one microphone badge at all? Just making sure there's no issues.
I ran into a UI bug where if I refreshed after enabling microphone and camera, there would be 2 microphones, or an incorrect icon.
I am not sure how you get to a state where you add more than one camera badge, it should either be one camera badge or zero camera badges (unless you are referring to adding a camera badge and a microphone badge).
As for refreshing, this should automatically revoke the camera or microphone permission and all the badges should disappear. I am not able to reproduce the bug you are describing. Can you share more info and/or reproduction steps?
The only issue I have been running into is adding a second permission badge after the first infobar is dismissed. The issue is that both camera and microphone permissions share the same underlying infobar type (InfobarType::kInfobarTypePermissions), but we are trying to create separate badges for each permission. I updated updateBadgesShownForWebState to fix this issue by making separate calls to updateDisplayedBadge for each badge.
Ok sounds good, it was just an old problem I found which was probably resolved with this cleaner CL. So I'm ok with not digging into this if you can't repro
Related details
- Ibrahim Kanouche
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 1 comment![]( "Open in Gerrit")
- Gauthier Ambard
- Ibrahim Kanouche
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 1 comment![]( "Open in Gerrit")
- Ibrahim Kanouche
- Sergio Collazos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |