[ios] Refactor GeminiCapabilitiesManager to observe GeminiService [chromium/src : main]
Joemer Ramos (Gerrit)
Joemer Ramos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Commit-Queue | +1 |
1 comment
Related details
- Adam Arcaro
- Yasaman Sedaghat
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 1 comment![]( "Open in Gerrit")
Thanks for the quick improvement here! I think this is a much better direction, keeping the responsibility to track eligibility in the GeminiService
However I looked through the `GeminiService` and don't think that `OnGeminiEligibilityChanged` is being called everywhere that it should. For example, I don't think sign outs are being included
Can you please look through it and add any calls you find to be missing? Thanks!
Related details
- Joemer Ramos
- Yasaman Sedaghat
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")
Thanks for the quick improvement here! I think this is a much better direction, keeping the responsibility to track eligibility in the GeminiService
However I looked through the `GeminiService` and don't think that `OnGeminiEligibilityChanged` is being called everywhere that it should. For example, I don't think sign outs are being included
Can you please look through it and add any calls you find to be missing? Thanks!
Isn't sign outs covered by [OnPrimaryAccountChanged](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/intelligence/bwg/model/gemini_service_impl.mm;l=127)? And in general it looks like `CheckGeminiEnterpriseEligibility();` is called in a bunch of good places to properly track eligibility which eventually call `OnGeminiEligibilityChanged`
Related details
- Adam Arcaro
- Yasaman Sedaghat
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. |
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yasaman Sedaghat
| Code-Review | +1 |
2 comments
Please wait for Adam's lgtm as well, thanks!
NSUserDefaults* shared_defaults,should shared_defaults and capabilities be private variables instead?
Related details
- Adam Arcaro
- 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")
Please wait for Adam's lgtm as well, thanks!
Sg thanks!
NSUserDefaults* shared_defaults,should shared_defaults and capabilities be private variables instead?
I'd prefer to keep them as parameters here. Since shared_defaults can be modified across 1P apps/extensions, fetching and passing the most up-to-date objects during the function's execution lifecycle ensures we don't accidentally rely on a cached or stale local state. It also keeps this utility method pure and decoupled, making it much easier to unit test.
Related details
- Adam Arcaro
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")
Joemer RamosThanks for the quick improvement here! I think this is a much better direction, keeping the responsibility to track eligibility in the GeminiService
However I looked through the `GeminiService` and don't think that `OnGeminiEligibilityChanged` is being called everywhere that it should. For example, I don't think sign outs are being included
Can you please look through it and add any calls you find to be missing? Thanks!
Isn't sign outs covered by [OnPrimaryAccountChanged](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/intelligence/bwg/model/gemini_service_impl.mm;l=127)? And in general it looks like `CheckGeminiEnterpriseEligibility();` is called in a bunch of good places to properly track eligibility which eventually call `OnGeminiEligibilityChanged`
I don't think we need to add any OnGeminiEligibilityChanged calls, feel free to reopen if you disagree.
Related details
- Adam Arcaro
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 1 comment![]( "Open in Gerrit")
Joemer RamosThanks for the quick improvement here! I think this is a much better direction, keeping the responsibility to track eligibility in the GeminiService
However I looked through the `GeminiService` and don't think that `OnGeminiEligibilityChanged` is being called everywhere that it should. For example, I don't think sign outs are being included
Can you please look through it and add any calls you find to be missing? Thanks!
Joemer RamosIsn't sign outs covered by [OnPrimaryAccountChanged](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/intelligence/bwg/model/gemini_service_impl.mm;l=127)? And in general it looks like `CheckGeminiEnterpriseEligibility();` is called in a bunch of good places to properly track eligibility which eventually call `OnGeminiEligibilityChanged`
I don't think we need to add any OnGeminiEligibilityChanged calls, feel free to reopen if you disagree.
Technically the eligibility has changed prior to resolving `CheckGeminiEnterpriseEligibility`. This is an async call, so if it takes a long time to respond, the user will retain their previous eligibility even if their new account is already deemed ineligible
Let me know if you think this makes sense to consider
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 added 1 comment![]( "Open in Gerrit")
Joemer RamosThanks for the quick improvement here! I think this is a much better direction, keeping the responsibility to track eligibility in the GeminiService
However I looked through the `GeminiService` and don't think that `OnGeminiEligibilityChanged` is being called everywhere that it should. For example, I don't think sign outs are being included
Can you please look through it and add any calls you find to be missing? Thanks!
Joemer RamosIsn't sign outs covered by [OnPrimaryAccountChanged](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/intelligence/bwg/model/gemini_service_impl.mm;l=127)? And in general it looks like `CheckGeminiEnterpriseEligibility();` is called in a bunch of good places to properly track eligibility which eventually call `OnGeminiEligibilityChanged`
Adam ArcaroI don't think we need to add any OnGeminiEligibilityChanged calls, feel free to reopen if you disagree.
Technically the eligibility has changed prior to resolving `CheckGeminiEnterpriseEligibility`. This is an async call, so if it takes a long time to respond, the user will retain their previous eligibility even if their new account is already deemed ineligible
Let me know if you think this makes sense to consider
Yeah that makes sense to consider the sign out case and in general have an updated eligibility while waiting for the async call to respond. I've added an additional check for signed out and enterprise users, let me know if it makes sense.
Related details
- Adam Arcaro
- Yasaman Sedaghat
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")
@yasa...@google.com can you verify that this extra Gemini checks are good? I'm confident about the signed out user check, but not sure about the other ones. But seems like they're generally good for keeping Gemini eligibility updated with sign in status
Joemer Ramos (Gerrit)
Joemer Ramos voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Adam Arcaro
- Yasaman Sedaghat
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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Yasaman Sedaghat (Gerrit)
Yasaman Sedaghat added 1 comment![]( "Open in Gerrit")
void GeminiServiceImpl::OnExtendedAccountInfoUpdated(in all of these new methods, shouldn't you be calling CheckGeminiEnterpriseEligibility() which will eventually call the observers if the eligibility changes (similar to OnRefreshTokenUpdatedForAccount)?
Related details
- Adam Arcaro
- 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. |