[iOS] Integrate Gemini Fullscreen Promo with Feature Engagement Toolkit [chromium/src : main]
Adam Arcaro (Gerrit)
Adam Arcaro added 1 comment![]( "Open in Gerrit")
Joemer and Nic should have the context to review this while I'm OOO. Please reach out to me if anything is unclear and I'll help when I'm available, thanks!
Related details
- Joemer Ramos
- Nicolas MacBeth
- Tarek Makkouk
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 8 comments![]( "Open in Gerrit")
Thanks for the work! Can you also add histogram metrics for the FET. You get this for free by adding variations in xml files. Check out [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7087869) as a reference.
config.trigger = EventConfig("gemini_fullscreen_promo_triggered",Add this as a string constant in components/feature_engagement/public/event_constants.h
EventConfig("gemini_consent_given", Comparator(EQUAL, 0), 3650, 3650);You can use `feature_engagement::kMaxStoragePeriod` if you want this to only check once. Unless there's a reason we're doing 3650 days?
EventConfig("gemini_consent_given", Comparator(EQUAL, 0), 3650, 3650);Add a string constant here too.
if (kIPHiOSGeminiContextualCueChip.name == feature->name) {There's already an iOS build flag guard in this file (like here), move your code to somewhere within the existing flag guard.
&kIPHiOSGeminiFullscreenPromoFeature,Add this to the header file `feature_list.h`
->NotifyEvent("gemini_consent_given");Replace this with the constant you make.
"+ios/chrome/browser/feature_engagement/model/tracker_factory.h",While you're here, can you make this list alphabetical?
Related details
- Nicolas MacBeth
- Tarek Makkouk
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 1 comment![]( "Open in Gerrit")
One more, also, note and fix the test cases before your next patch!
Tarek Makkouk (Gerrit)
Tarek Makkouk added 9 comments![]( "Open in Gerrit")
Thanks for the work! Can you also add histogram metrics for the FET. You get this for free by adding variations in xml files. Check out [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7087869) as a reference.
Done
One more, also, note and fix the test cases before your next patch!
Done
config.trigger = EventConfig("gemini_fullscreen_promo_triggered",Add this as a string constant in components/feature_engagement/public/event_constants.h
Done
EventConfig("gemini_consent_given", Comparator(EQUAL, 0), 3650, 3650);You can use `feature_engagement::kMaxStoragePeriod` if you want this to only check once. Unless there's a reason we're doing 3650 days?
Done
EventConfig("gemini_consent_given", Comparator(EQUAL, 0), 3650, 3650);Add a string constant here too.
Done
if (kIPHiOSGeminiContextualCueChip.name == feature->name) {There's already an iOS build flag guard in this file (like here), move your code to somewhere within the existing flag guard.
Done
Add this to the header file `feature_list.h`
Done
Replace this with the constant you make.
Done
"+ios/chrome/browser/feature_engagement/model/tracker_factory.h",While you're here, can you make this list alphabetical?
- Adam Arcaro
- 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 8 comments![]( "Open in Gerrit")
Thanks for the work, this looks good so far! Added some comments, let me know if you have any questions
config.used =
EventConfig(events::kIOSGeminiConsentGiven, Comparator(EQUAL, 0),
feature_engagement::kMaxStoragePeriod,
feature_engagement::kMaxStoragePeriod);I don't think this is how `config.used` is supposed to be used. Maybe we do something similar to:
```
// This feature should show for 2 weeks after the client was first
// considered eligible for the AI Hub.
config.event_configs.insert(
EventConfig(events::kIOSGeminiEligiblity, Comparator(EQUAL, 1), 14,
feature_engagement::kMaxStoragePeriod));
```
so we insert an event config, and then tie it to an event. So something like
```
// Some comment.
config.event_configs.insert(
EventConfig(events::kIOSGeminiConsentGiven, Comparator(EQUAL, 0),
feature_engagement::kMaxStoragePeriod,
feature_engagement::kMaxStoragePeriod));
```
base::FEATURE_DISABLED_BY_DEFAULT);Enabled by default please. This isn't a normal feature flag, this is a feature flag to enable the feature config in the FET system. We can enable by default since actually logging the events should be done behind an actual feature flag.
feature_engagement::TrackerFactory::GetForProfile(self.profile)
->NotifyEvent(
feature_engagement::events::kIOSGeminiFullscreenPromoTriggered);Please hide behind the feature flag: `IsGeminiNavigationPromoEnabled()`
feature_engagement::TrackerFactory::GetForProfile(
ProfileIOS::FromBrowserState(
_webStateList->GetActiveWebState()->GetBrowserState()))
->NotifyEvent(feature_engagement::events::kIOSGeminiConsentGiven);Hide behind the feature flag `IsGeminiNavigationPromoEnabled()`
auto* tracker = static_cast<feature_engagement::test::MockTracker*>(
feature_engagement::TrackerFactory::GetForProfile(profile_.get()));
EXPECT_CALL(*tracker,
ShouldTriggerHelpUI(testing::Ref(
feature_engagement::kIPHiOSGeminiFullscreenPromoFeature)))
.WillRepeatedly(testing::Return(true));Can we test with `tracker->NotifyEvent()` to emulate a user giving consent rather than returning true? Should be possible.
auto* tracker = static_cast<feature_engagement::test::MockTracker*>(
feature_engagement::TrackerFactory::GetForProfile(profile_.get()));
EXPECT_CALL(*tracker,
ShouldTriggerHelpUI(testing::Ref(
feature_engagement::kIPHiOSGeminiFullscreenPromoFeature)))
.WillRepeatedly(testing::Return(false));Same here, can we emulate an event just not being logged and the promo failing to show?
summary="showing the fullscreen Gemini in Chrome iOS promo"/>Nit: Don't mention Chrome, since this is technically in the chromium repo, iOS is good enough!
Related details
- Adam Arcaro
- Tarek Makkouk
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. |