[aim-drive] - Create a settings linkout for users to manage AIM consent [chromium/src : main]
Josiah Kiehl (Gerrit)
Josiah Kiehl voted and added 3 comments![]( "Open in Gerrit")
Votes added by Josiah Kiehl
| Code-Review | +1 |
3 comments
!profile->IsChild() && !enterprise_util::IsBrowserManaged(profile);This seems like logic that the strings provider shouldn't care about. Is there a better utility somewhere that could hold this evaluation, even as a static method? The strings provider should provide strings, so the logic for whether or not the profile is eligible for Drive should live somewhere Drive-specific, I think.
"https://myactivity.google.com/myactivity";Is there a reason some of these use utf 16 strings and others don't? It seems like more are than not, so perhaps we should follow the pattern? I asked jetski about it, and it said the following:
In modern cross-platform development (such as Chromium), UTF-16 is widely used for text representation (e.g., matching JavaScript/V8 strings, Java strings on Android, and Windows APIs). Using u"string" prevents conversion overhead when interacting with these APIs.
I don't personally know if it matters in this case, but I noticed the difference and perhaps just matching the pattern is wise.
assertTrue(!!row);isVisible tests if row is true anyway, so we probably only need the isVisibile assert.
Related details
- Adam Yao
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 Yao (Gerrit)
Adam Yao added 3 comments![]( "Open in Gerrit")
!profile->IsChild() && !enterprise_util::IsBrowserManaged(profile);This seems like logic that the strings provider shouldn't care about. Is there a better utility somewhere that could hold this evaluation, even as a static method? The strings provider should provide strings, so the logic for whether or not the profile is eligible for Drive should live somewhere Drive-specific, I think.
Yep - so we discussed this offline but I'll write it here to keep a record. For M151, we're going to unconditionally link to Connected Content Apps regardless of account type.
For M152, an improvement here would be to use `IdentityManager` and [iseligibleforpersonalcontextcontrols](https://g3doc.corp.google.com/identity/accountcapabilities/definitions/footprints/facs/iseligibleforpersonalcontextcontrols/README.md?cl=head) to synchronously check account eligibility if they can view pcontext settings.
see comment [here](https://docs.google.com/document/d/1oHDczZBOiTEQp4SAc82GeSdteeVyfWCmJ4pHWeN-iUk/edit?resourcekey=0-Jd1pCTzTKjw7F0LBYIbceQ&tab=t.0#:~:text=Explanation%20for%20why%20we%20unconditionally%20linking%20all%20account%20types%20for%20now%2C%20but%20how%20it%20will%20be%20improved%20in%20M152%2B%20as%20a%20fast%20follow.%C2%A0) for more explanation.
Is there a reason some of these use utf 16 strings and others don't? It seems like more are than not, so perhaps we should follow the pattern? I asked jetski about it, and it said the following:
In modern cross-platform development (such as Chromium), UTF-16 is widely used for text representation (e.g., matching JavaScript/V8 strings, Java strings on Android, and Windows APIs). Using u"string" prevents conversion overhead when interacting with these APIs.
I don't personally know if it matters in this case, but I noticed the difference and perhaps just matching the pattern is wise.
I think we should follow the existing pattern, added the u" " string. jetski makes sense to me, updated.
isVisible tests if row is true anyway, so we probably only need the isVisibile assert.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rainhard Findling (Gerrit)
Rainhard Findling added 1 comment![]( "Open in Gerrit")
<cr-link-row id="googleSearchAiModeWorkspaceRow" class="hr"I am surprised that this user-visible change is not behind a feature:
- Is it confirmed that this user-visible change can roll out through waterfall to all users, instead of going through a launch and Finch rollout?
- Is it confirmed this user-visible change doesn't even need a killswitch feature?
Related details
- Adam Yao
- Darryl James
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. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
browser_test looks good but will defer to @rainhard as the area OWNER before sign off; thanks!
Related details
- Adam Yao
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 Yao (Gerrit)
Adam Yao added 2 comments![]( "Open in Gerrit")
<cr-link-row id="googleSearchAiModeWorkspaceRow" class="hr"I am surprised that this user-visible change is not behind a feature:
- Is it confirmed that this user-visible change can roll out through waterfall to all users, instead of going through a launch and Finch rollout?
- Is it confirmed this user-visible change doesn't even need a killswitch feature?
Thanks for calling this out!
Yes, this change is approved/confirmed to roll out via waterfall to all users in M151 rather than through a progressive Finch experiment.
However, I agree it should have a killswitch feature flag for safety in case we ever need to disable the link-out remotely. I'll add a `FEATURE_ENABLED_BY_DEFAULT` killswitch flag and gate the row visibility.
assertTrue(!!row);Adam YaoisVisible tests if row is true anyway, so we probably only need the isVisibile assert.
Ok. removed. ty!
I actually had to add assertTrue(!!row); back in the latest patchset to satisfy TypeScript strict null checking during git cl format --js / autoninja.
Related details
- Rainhard Findling
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 Yao (Gerrit)
Adam Yao added 1 comment![]( "Open in Gerrit")
<cr-link-row id="googleSearchAiModeWorkspaceRow" class="hr"Adam YaoI am surprised that this user-visible change is not behind a feature:
- Is it confirmed that this user-visible change can roll out through waterfall to all users, instead of going through a launch and Finch rollout?
- Is it confirmed this user-visible change doesn't even need a killswitch feature?
Thanks for calling this out!
Yes, this change is approved/confirmed to roll out via waterfall to all users in M151 rather than through a progressive Finch experiment.
However, I agree it should have a killswitch feature flag for safety in case we ever need to disable the link-out remotely. I'll add a `FEATURE_ENABLED_BY_DEFAULT` killswitch flag and gate the row visibility.
- Josiah Kiehl
- Juliet Lévesque
- Rainhard Findling
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. |
Rainhard Findling (Gerrit)
Rainhard Findling voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rainhard Findling
| Code-Review | +1 |
2 comments
ai_page/ and its tests, icons, and string_provider LGTM % double-checking that this is OK product-wise
<cr-link-row id="googleSearchAiModeWorkspaceRow" class="hr"Adam YaoI am surprised that this user-visible change is not behind a feature:
- Is it confirmed that this user-visible change can roll out through waterfall to all users, instead of going through a launch and Finch rollout?
- Is it confirmed this user-visible change doesn't even need a killswitch feature?
Adam YaoThanks for calling this out!
Yes, this change is approved/confirmed to roll out via waterfall to all users in M151 rather than through a progressive Finch experiment.
However, I agree it should have a killswitch feature flag for safety in case we ever need to disable the link-out remotely. I'll add a `FEATURE_ENABLED_BY_DEFAULT` killswitch flag and gate the row visibility.
wdyt?
Code-wise that LGTM, but I want to point out that this being unconditionally shown to _all_ users strikes me as odd. I'll approve the CL - but recommend y'all to double-check the product POV of this with the respective product owners (of the AI page --> @andre...@google.com).
Related details
- Adam Yao
- Josiah Kiehl
- Juliet Lévesque
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
1 comment
lgtm for settings_ui.*
Related details
- Adam Yao
- Josiah Kiehl
- Juliet Lévesque
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl voted and added 4 comments![]( "Open in Gerrit")
Votes added by Josiah Kiehl
| Code-Review | +1 |
4 comments
declare private showGoogleSearchAiModeWorkspaceControl_: boolean;For a followup: other entries have interaction metrics that we might want to also add for the drive linkout.
OpenWindowProxyImpl.getInstance().openUrl(We might want to add a this.recordInteractionMetrics_ in here. Fine for a followup.
#include "components/signin/public/identity_manager/identity_manager.h"might be unused.
void AddAiStrings(content::WebUIDataSource* html_source, Profile* profile) {profile is unused.
Related details
- Adam Yao
- Juliet Lévesque
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 Yao (Gerrit)
Adam Yao added 4 comments![]( "Open in Gerrit")
declare private showGoogleSearchAiModeWorkspaceControl_: boolean;For a followup: other entries have interaction metrics that we might want to also add for the drive linkout.
noted. adding to fast follow.
We might want to add a this.recordInteractionMetrics_ in here. Fine for a followup.
will add to fast follow. noted.
#include "components/signin/public/identity_manager/identity_manager.h"Adam Yaomight be unused.
removed.
void AddAiStrings(content::WebUIDataSource* html_source, Profile* profile) {Adam Yaoprofile is unused.
ah sorry, meant to remove that, ty!
Related details
- Juliet Lévesque
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Yao (Gerrit)
Adam Yao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Adam Yao (Gerrit)
Adam Yao voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Adam Yao (Gerrit)
Adam Yao added 1 comment![]( "Open in Gerrit")
Holding off on until @andre...@google.com takes a look here. The context is here
Related details
- Juliet Lévesque
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. |