[iOS] Create Best Features Best of FRE flag [chromium/src : main]
Briana McClure (Gerrit)
Briana McClure added 1 comment![]( "Open in Gerrit")
Hey Hira, PTAL. cc: Robbie for visibility with the Best of App FRE implementation.
Related details
- Hira Mahmood
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hira Mahmood (Gerrit)
Hira Mahmood added 1 comment![]( "Open in Gerrit")
if (IsBestOfAppBestFeaturesEnabled()) {
return BestFeaturesScreenVariationType::kGeneralScreenAfterDBPromo;
}I believe the way things are set up right now, it would be possible for the screen to be added twice.
If we return `kGeneralScreenAfterDBPromo` in this function, then this line here (https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/first_run/coordinator/first_run_screen_provider.mm;l=54) will add the best features screen if kUpdatedFirstRunSequence is disabled (which it is by default, but might be enabled locally for you). And then again we'll add the screen bc of line 119 in `first_run_screen_provider.mm` in this CL.
Related details
- Briana McClure
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. |
Briana McClure (Gerrit)
Briana McClure added 1 comment![]( "Open in Gerrit")
if (IsBestOfAppBestFeaturesEnabled()) {
return BestFeaturesScreenVariationType::kGeneralScreenAfterDBPromo;
}I believe the way things are set up right now, it would be possible for the screen to be added twice.
If we return `kGeneralScreenAfterDBPromo` in this function, then this line here (https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/first_run/coordinator/first_run_screen_provider.mm;l=54) will add the best features screen if kUpdatedFirstRunSequence is disabled (which it is by default, but might be enabled locally for you). And then again we'll add the screen bc of line 119 in `first_run_screen_provider.mm` in this CL.
That's a good point! I removed the changes from this helper and instead updated Best Features Mediator and View Controller to handle the case for when IsBestOfAppBestFeaturesEnabled().
Related details
- Hira Mahmood
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. |
Hira Mahmood (Gerrit)
Hira Mahmood added 1 comment![]( "Open in Gerrit")
if (IsBestOfAppBestFeaturesEnabled()) {
return BestFeaturesScreenVariationType::kGeneralScreenAfterDBPromo;
}Briana McClureI believe the way things are set up right now, it would be possible for the screen to be added twice.
If we return `kGeneralScreenAfterDBPromo` in this function, then this line here (https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/first_run/coordinator/first_run_screen_provider.mm;l=54) will add the best features screen if kUpdatedFirstRunSequence is disabled (which it is by default, but might be enabled locally for you). And then again we'll add the screen bc of line 119 in `first_run_screen_provider.mm` in this CL.
That's a good point! I removed the changes from this helper and instead updated Best Features Mediator and View Controller to handle the case for when IsBestOfAppBestFeaturesEnabled().
I think it's a bit confusing to have `GetBestFeaturesScreenVariationType()` not return the correct variation in all cases (for example this function could return disabled while IsBestOfAppBestFeaturesEnabled() returns true, and the screen is still shown).
Wdyt of instead adding a new enum to `BestFeaturesScreenVariationType` called `kBestOfAppVariation` or something similar? And then in `AddDBPromoAndBestFeaturesScreens()` we avoid adding the screen for this new enum case. This would keep the variant flow intact and lets us clean up the `if (IsBestOfAppBestFeaturesEnabled())` conditional checks in the UI layout layers
Related details
- Briana McClure
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. |