[iOS] Add Gemini FRE UI components [chromium/src : main]
Joemer Ramos (Gerrit)
Joemer Ramos added 23 comments![]( "Open in Gerrit")
Apologies for the large amount of comments and that you have to resolve each one by one, but just wanted to make sure that instances found of BWG were marked and addressed
// Dismisses the BWG flow with a completion block before stopping thenit: Gemini
// Coordinator that manages the first run and any BWG triggers.nit: Gemini
// Mediator for handling all logic related to BWG.nit: Gemini First Run Promo
BWGFREWrapperViewController* _FREWrapperViewController;Since this coordinator is now specific to FRE, you can just rename this to `_viewController` as the FREWrapper is implied.
// Handler for sending BWG commands.nit:Gemini
id<BWGCommands> _BWGCommandsHandler;nit: Rename to `_geminiCommandsHandler`.
[self dismissBWGFromOtherWindowsWithCompletion:^() {This is fine for now, but can't we also move this functionality to `GeminiBrowserAgent`?
- (void)dismissBWGConsentUIWithCompletion:(void (^)())completion {nit: replace BWG with Gemini
// Starts the BWG coordinator.nit: GeminiFirstRunCoordinator
// Returns the currently active WebState's BWG tab helper.nit: Gemini
- (BwgTabHelper*)activeWebStateBWGTabHelper {nit:Gemini
- (void)dismissBWGFromOtherWindowsWithCompletion:(ProceduralBlock)completion {nit: Gemini
// Dismiss BWG in all the other browsers for all profiles.nit: Gemini
id<BWGCommands> BWGCommandsHandler =nit: Gemini
raw_ptr<BwgService> _BWGService;nit:`_geminiService`
raw_ptr<BwgBrowserAgent> _BwgBrowserAgent;nit: _geminiBrowserAgent
BWGService:(BwgService*)BWGServicenit: Gemini
#pragma mark - BWGConsentMutatorall these BWG instances you can leave, as BWGConsentMutator being refactored to GeminiFREConsentMutator should be a separate CL. Feel free to resolve after you see this
// Notifies the currently active WebState's BWG tab helper that the FRE will benit: Gemini
- (void)FREWillBeBackgrounded {can we rename to `prepareFREBackground`?
// Fetches zero-state suggestions from the BWG tab helper and pass them to thenit: Gemini but im guessing this method will be moved soon to `GeminiBrowserAgent`?
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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 16 comments![]( "Open in Gerrit")
- (instancetype)initWithBaseViewController:(UIViewController*)viewControllernit: Please add a comment to cover the extra params, such as `entryPoint` and `completion`
[self dismissBWGFromOtherWindowsWithCompletion:^() {Not for this CL, but now that we're moving the floaty presentation to the browser agent, let's make sure to dismiss the floaty on other windows in there too. This line will only affect FREs now
You can add a TODO for tracking
ios::provider::ResetGemini();We can move this to the browser agent since the FRE doesn't show Gemini anymore
// Starts the BWG coordinator.`Starts the Gemini FRE coordinator.`
[_mediator updateFRECompletionHandler:_completion];Similar to the coordinator, if we expect the mediator to always need a completion handler, then let's put it in its constructor
initWithPromo:showPromonit: Just use `_mediator.shouldShowPromo` inline here
BwgTabHelper* BWGTabHelper = [self activeWebStateBWGTabHelper];Please add a TODO to remove all this animation background logic, we won't need it for the FRE (and maybe not even for the floaty with copresence)
_baseViewController = baseViewController;This isn't being used, let's remove it
}Most stuff in this function shouldn't fire off when the completion handler property is set. Things like notifying events, logging metrics etc are probably results of UI events (like the FRE being shown, next page being pressed etc). It would be best to have the VC notify the mediator of these events through a `mutator`, and then the mediator can call these events
- (BOOL)shouldShowConsent {With this refactor, starting the FRE coordinator should always present some UI, so at the very least the consent page will always be shown. We can remove this function and condition entirely
- (BOOL)shouldShowAIHubIPH {This is repeated here and in the coordinator
Maybe @joeme...@google.com has thoughts on where this should live?
- (void)executeZeroStateSuggestions {This will be called by the BrowserAgent when the overlay is shown, so we can remove it here
Related details
- 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. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 37 comments![]( "Open in Gerrit")
// Dismisses the BWG flow with a completion block before stopping theTarek Makkouknit: Gemini
Done
// Coordinator that manages the first run and any BWG triggers.Tarek Makkouknit: Gemini
Done
- (instancetype)initWithBaseViewController:(UIViewController*)viewControllernit: Please add a comment to cover the extra params, such as `entryPoint` and `completion`
Done
nit: Gemini First Run Promo
Done
Since this coordinator is now specific to FRE, you can just rename this to `_viewController` as the FREWrapper is implied.
Done
// Handler for sending BWG commands.Tarek Makkouknit:Gemini
Done
nit: Rename to `_geminiCommandsHandler`.
Done
[self dismissBWGFromOtherWindowsWithCompletion:^() {Not for this CL, but now that we're moving the floaty presentation to the browser agent, let's make sure to dismiss the floaty on other windows in there too. This line will only affect FREs now
You can add a TODO for tracking
Done
This is fine for now, but can't we also move this functionality to `GeminiBrowserAgent`?
Added a TODO for the next CL.
ios::provider::ResetGemini();We can move this to the browser agent since the FRE doesn't show Gemini anymore
Removed, I'll have it in the browser agent in the next CL.
- (void)dismissBWGConsentUIWithCompletion:(void (^)())completion {nit: replace BWG with Gemini
Done
// Starts the BWG coordinator.`Starts the Gemini FRE coordinator.`
Done
// Starts the BWG coordinator.Tarek Makkouknit: GeminiFirstRunCoordinator
Done
[self prepareAIHubIPH];Tarek MakkoukRemove the duplicate
Done
[_mediator updateFRECompletionHandler:_completion];Similar to the coordinator, if we expect the mediator to always need a completion handler, then let's put it in its constructor
Done
initWithPromo:showPromonit: Just use `_mediator.shouldShowPromo` inline here
Done
BwgTabHelper* BWGTabHelper = [self activeWebStateBWGTabHelper];Please add a TODO to remove all this animation background logic, we won't need it for the FRE (and maybe not even for the floaty with copresence)
Done
// Returns the currently active WebState's BWG tab helper.Tarek Makkouknit: Gemini
Done
- (BwgTabHelper*)activeWebStateBWGTabHelper {Tarek Makkouknit:Gemini
Done
- (void)dismissBWGFromOtherWindowsWithCompletion:(ProceduralBlock)completion {Tarek Makkouknit: Gemini
Done
// Dismiss BWG in all the other browsers for all profiles.Tarek Makkouknit: Gemini
Done
id<BWGCommands> BWGCommandsHandler =Tarek Makkouknit: Gemini
Done
raw_ptr<BwgService> _BWGService;Tarek Makkouknit:`_geminiService`
Done
raw_ptr<BwgBrowserAgent> _BwgBrowserAgent;Tarek Makkouknit: _geminiBrowserAgent
Done
BWGService:(BwgService*)BWGServiceTarek Makkouknit: Gemini
Done
_baseViewController = baseViewController;This isn't being used, let's remove it
Done
_BWGService = BWGService;Tarek Makkoukditto
Done
_BwgBrowserAgent = BwgBrowserAgent;Tarek Makkoukditto
Done
Most stuff in this function shouldn't fire off when the completion handler property is set. Things like notifying events, logging metrics etc are probably results of UI events (like the FRE being shown, next page being pressed etc). It would be best to have the VC notify the mediator of these events through a `mutator`, and then the mediator can call these events
Done
- (BOOL)shouldShowConsent {With this refactor, starting the FRE coordinator should always present some UI, so at the very least the consent page will always be shown. We can remove this function and condition entirely
Done
- (BOOL)shouldShowAIHubIPH {This is repeated here and in the coordinator
Maybe @joeme...@google.com has thoughts on where this should live?
I think it makes sense to keep it in the mediator given that the coordinator owns the mediator, so I just put it in the coordinator as:
```
// Returns whether to show AI Hub IPH.
- (BOOL)shouldShowAIHubIPH {
return _mediator.shouldShowAIHubIPH;
}
```
@joeme...@google.com Please feel free to mark as unresolved if you disagree.
all these BWG instances you can leave, as BWGConsentMutator being refactored to GeminiFREConsentMutator should be a separate CL. Feel free to resolve after you see this
Seen
// Notifies the currently active WebState's BWG tab helper that the FRE will beTarek Makkouknit: Gemini
Done
can we rename to `prepareFREBackground`?
Done
// Fetches zero-state suggestions from the BWG tab helper and pass them to thenit: Gemini but im guessing this method will be moved soon to `GeminiBrowserAgent`?
Done
- (void)executeZeroStateSuggestions {This will be called by the BrowserAgent when the overlay is shown, so we can remove it here
Done
Related details
- 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 4 comments![]( "Open in Gerrit")
[GeminiCommandsHandler dismissGeminiFlowWithCompletion:^() {nit: `geminiCommandsHandler`
[mediator viewDidAppear];You can just do an `OCMExpect([mediator functionName])` and then a `OCMVerify(mediator)`. This will check that at some point, the mediator function was called. Generally, we should be able to write tests without having to find ways to access private variables. If we find ourselves searching for private variables often, we should rethink our design as it means the design itself is not easily testable.
- (BOOL)shouldShowAIHubIPH {Tarek MakkoukThis is repeated here and in the coordinator
Maybe @joeme...@google.com has thoughts on where this should live?
I think it makes sense to keep it in the mediator given that the coordinator owns the mediator, so I just put it in the coordinator as:
```
// Returns whether to show AI Hub IPH.
- (BOOL)shouldShowAIHubIPH {
return _mediator.shouldShowAIHubIPH;
}
```@joeme...@google.com Please feel free to mark as unresolved if you disagree.
Marked as unresolved. I think we should remove the mediator version. Triggering the AI Hub IPH (deciding and sending a trigger for non-Gemini FRE UI) sounds like something a coordinator would do rather than computing model logic within the FRE (what a mediator should handle more or less)
- (void)viewDidAppear {Mediator function names should be related to model logic rather than a UI update. Nit: Rename to something like `promoShown`, `notifyPromoShown`, or `updateModel`.
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. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 4 comments![]( "Open in Gerrit")
[GeminiCommandsHandler dismissGeminiFlowWithCompletion:^() {Tarek Makkouknit: `geminiCommandsHandler`
Done
You can just do an `OCMExpect([mediator functionName])` and then a `OCMVerify(mediator)`. This will check that at some point, the mediator function was called. Generally, we should be able to write tests without having to find ways to access private variables. If we find ourselves searching for private variables often, we should rethink our design as it means the design itself is not easily testable.
Updated the tests to verify observable public behavior without having to access internal variables.
- (BOOL)shouldShowAIHubIPH {Tarek MakkoukThis is repeated here and in the coordinator
Maybe @joeme...@google.com has thoughts on where this should live?
Joemer RamosI think it makes sense to keep it in the mediator given that the coordinator owns the mediator, so I just put it in the coordinator as:
```
// Returns whether to show AI Hub IPH.
- (BOOL)shouldShowAIHubIPH {
return _mediator.shouldShowAIHubIPH;
}
```@joeme...@google.com Please feel free to mark as unresolved if you disagree.
Marked as unresolved. I think we should remove the mediator version. Triggering the AI Hub IPH (deciding and sending a trigger for non-Gemini FRE UI) sounds like something a coordinator would do rather than computing model logic within the FRE (what a mediator should handle more or less)
Done
Mediator function names should be related to model logic rather than a UI update. Nit: Rename to something like `promoShown`, `notifyPromoShown`, or `updateModel`.
- 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. |
Adam Arcaro (Gerrit)
Adam Arcaro added 5 comments![]( "Open in Gerrit")
- (void)didCloseBWGPromo {Please rename all of these from BWG -> Gemini
}If `notifyPromoShown` is called, we should assume that the promo was actually show and therefore shouldn't need to check this. We can just call `[self logPromoShown]`
- (void)notifyPromoShown;For consistency with other functions, let's name this `didShowGeminiPromo`
[self.mutator notifyPromoShown];I think this is also called when the consent page is shown, right? (like in the case where the user has already seen the promo 3x)
We can probably check `_currentChildViewController` here
Related details
- Joemer Ramos
- 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 3 comments![]( "Open in Gerrit")
// Returns the `_entryPoint` the coordinator was intialized from.nit:initialized
// Gemini Mediator.nit: Gemini First Run Mediator.
base::TimeTicks _BWGOverlayPreparationStartTime;nit: Same as Adam's comment to rename BWG to Gemini
Related details
- 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. |
Tarek Makkouk (Gerrit)
Tarek Makkouk added 7 comments![]( "Open in Gerrit")
// Returns the `_entryPoint` the coordinator was intialized from.Tarek Makkouknit:initialized
Done
nit: Gemini First Run Mediator.
Done
nit: Same as Adam's comment to rename BWG to Gemini
Done
Please rename all of these from BWG -> Gemini
Done
If `notifyPromoShown` is called, we should assume that the promo was actually show and therefore shouldn't need to check this. We can just call `[self logPromoShown]`
Done
For consistency with other functions, let's name this `didShowGeminiPromo`
Done
I think this is also called when the consent page is shown, right? (like in the case where the user has already seen the promo 3x)
We can probably check `_currentChildViewController` here
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Code-Review | +1 |
1 comment
Related details
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Arcaro
| Code-Review | +1 |
1 comment
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Code-Review | +1 |
lgtm
- Adam Arcaro
- Tarek Makkouk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |