Add view for sign-in promo from Search AI Mode results [chromium/src : main]
Ryan Sultanem (Gerrit)
Ryan Sultanem added 4 comments![]( "Open in Gerrit")
Ryan SultanemHi both @ame...@google.com and @rs...@google.com, I see that both of your have worked / reviewed before changes related to the sign-in promo bubbles (for example bookmark_bubble_view which which I used as my blue print).
Whoever of you has the capacity, could you take a look in this (large) CL?
I took a quick look and left high level comments that requires Amelie's input (as she is more familiar with the details of this code).
If possible, it could be clearer to separate the AccessPoint addition into a different CL. That would helpful!
case signin_metrics::AccessPoint::kSearchAIModeBubble:
// The AI search mode promo does not relate to to the syncing of any data
// types.
return std::nullopt;This makes me wonder if this is the right approach - combining the logic directly with the existing access points/data types. Probably @ame...@google.com could provide some more context.
Adding it through this logic comes with some side effects:
- The dismiss limit will be considered as part of the global limits for the promos considered here (if not part of the SigninLimitsExperiments, which is just an experiment and will probably not launch). Is that ok?
- The flow is tied to a DataType, which at the end, tries to enable and migrate the data. Which does not seem to be the case for the AI search mode.
- Potentially other limitations?
I am just trying to ensure that there is an actual benefit in sharing the code (because it comes with conditional logic for the new access point). If there is value in sharing, that is great! Otherwise it seems like this adding some complexity to an existing flow.
Just making sure I have the complete picture here!
case signin::SignInPromoType::kSearchAIMode:
// TODO(crbug.com/486858498): Implement prefs for rate limiting.
break;Note; this is part of an experiment related to SigninPromoLimits. Check the start of the function. Which basically makes the dismiss count per data type instead of global (global to all data type promos).
You may find other places where there will be conditional checks based on the feature flag activation or not.
Related details
- Amelie Schneider
- Anthi Orfanou
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. |
Amelie Schneider (Gerrit)
Amelie Schneider added 2 comments![]( "Open in Gerrit")
Ryan SultanemHi both @ame...@google.com and @rs...@google.com, I see that both of your have worked / reviewed before changes related to the sign-in promo bubbles (for example bookmark_bubble_view which which I used as my blue print).
Whoever of you has the capacity, could you take a look in this (large) CL?
I took a quick look and left high level comments that requires Amelie's input (as she is more familiar with the details of this code).
If possible, it could be clearer to separate the AccessPoint addition into a different CL. That would helpful!
+1 for adding the access point in a separate CL
Thanks Anthi! I will take a deeper look, just wanted to leave this comment for now :)
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 4 comments![]( "Open in Gerrit")
Ryan SultanemHi both @ame...@google.com and @rs...@google.com, I see that both of your have worked / reviewed before changes related to the sign-in promo bubbles (for example bookmark_bubble_view which which I used as my blue print).
Whoever of you has the capacity, could you take a look in this (large) CL?
Amelie SchneiderI took a quick look and left high level comments that requires Amelie's input (as she is more familiar with the details of this code).
If possible, it could be clearer to separate the AccessPoint addition into a different CL. That would helpful!
+1 for adding the access point in a separate CL
Done, good point, I've moved the access point to https://chromium-review.git.corp.google.com/c/chromium/src/+/7616929
case signin_metrics::AccessPoint::kSearchAIModeBubble:
// The AI search mode promo does not relate to to the syncing of any data
// types.
return std::nullopt;This makes me wonder if this is the right approach - combining the logic directly with the existing access points/data types. Probably @ame...@google.com could provide some more context.
Adding it through this logic comes with some side effects:
- The dismiss limit will be considered as part of the global limits for the promos considered here (if not part of the SigninLimitsExperiments, which is just an experiment and will probably not launch). Is that ok?
- The flow is tied to a DataType, which at the end, tries to enable and migrate the data. Which does not seem to be the case for the AI search mode.
- Potentially other limitations?
I am just trying to ensure that there is an actual benefit in sharing the code (because it comes with conditional logic for the new access point). If there is value in sharing, that is great! Otherwise it seems like this adding some complexity to an existing flow.
Just making sure I have the complete picture here!
The reason I looked into the BubbleSignInPromoView as a building block because it provides us already with the the UI to use (everything within the yellow block https://screenshot.googleplex.com/5w27t29G7P97iQG from directly from re-using this view) including handful helpers for the account state and the triggering of the sign-in logic from the button.
I think the benefits we get from this re use are already enough.
I noticed the delegate focuses on the sync data types handing (although the name and the description are more generic).
One approach I can think of, is to produce another delegate for my use case (and extend the existing one).
case signin::SignInPromoType::kSearchAIMode:
// TODO(crbug.com/486858498): Implement prefs for rate limiting.
break;Note; this is part of an experiment related to SigninPromoLimits. Check the start of the function. Which basically makes the dismiss count per data type instead of global (global to all data type promos).
You may find other places where there will be conditional checks based on the feature flag activation or not.
For now I have added this comment in all the places that we could potentially need to adapt for the rate limits. It might be the case than we don't need them all, as this new promo comes with it's own rate limits requirements from the PRD.
I marked all those places because I want to revisit them in the upcoming change where I will focus on the rate limits.
// Copyright 2026 The Chromium AuthorsAnthi OrfanouI think I should move them to a more suitable location.
chrome/browser/ui/signin/promos/? Other recomendations?
I've made a new search_ai_mode/ dir in ui/views but let me knwow about other potential locations.
Related details
- Amelie Schneider
- Ryan Sultanem
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. |
Amelie Schneider (Gerrit)
Amelie Schneider added 3 comments![]( "Open in Gerrit")
case signin_metrics::AccessPoint::kSearchAIModeBubble:
// The AI search mode promo does not relate to to the syncing of any data
// types.
return std::nullopt;Anthi OrfanouThis makes me wonder if this is the right approach - combining the logic directly with the existing access points/data types. Probably @ame...@google.com could provide some more context.
Adding it through this logic comes with some side effects:
- The dismiss limit will be considered as part of the global limits for the promos considered here (if not part of the SigninLimitsExperiments, which is just an experiment and will probably not launch). Is that ok?
- The flow is tied to a DataType, which at the end, tries to enable and migrate the data. Which does not seem to be the case for the AI search mode.
- Potentially other limitations?
I am just trying to ensure that there is an actual benefit in sharing the code (because it comes with conditional logic for the new access point). If there is value in sharing, that is great! Otherwise it seems like this adding some complexity to an existing flow.
Just making sure I have the complete picture here!
The reason I looked into the BubbleSignInPromoView as a building block because it provides us already with the the UI to use (everything within the yellow block https://screenshot.googleplex.com/5w27t29G7P97iQG from directly from re-using this view) including handful helpers for the account state and the triggering of the sign-in logic from the button.
I think the benefits we get from this re use are already enough.
I noticed the delegate focuses on the sync data types handing (although the name and the description are more generic).
One approach I can think of, is to produce another delegate for my use case (and extend the existing one).
I agree with reusing the existing `BubbleSignInPromoView`, as visually they are very similar. However, it seems like `BubbleSignInPromoDelegate::OnSignIn` will have to be quite different - assuming that we will need to pass another callback instead of `SelectTypeAndMigrateLocalDataItemsWhenActive`.
I think I like the idea of branching out `OnSignIn()` based on the `access_point_`, e.g. after the call to `SignInFromSingleAccountPromo()`. Another delegate is also an option but I think that for now it is not too big, and the duplicated code also quite minimal when you exclude everything that has to do with data types.
So something like `MaybeMoveDataTypeAfterSignIn()` and `MaybeOpenAIModeAfterSignIn()` ?
// Copyright 2026 The Chromium AuthorsAnthi OrfanouI think I should move them to a more suitable location.
chrome/browser/ui/signin/promos/? Other recomendations?
I've made a new search_ai_mode/ dir in ui/views but let me knwow about other potential locations.
I think this is a good place, `chrome/browser/ui/signin/promos` has only type/access point agnostic classes.
Related details
- Anthi Orfanou
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. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 1 comment![]( "Open in Gerrit")
case signin_metrics::AccessPoint::kSearchAIModeBubble:
// The AI search mode promo does not relate to to the syncing of any data
// types.
return std::nullopt;Anthi OrfanouThis makes me wonder if this is the right approach - combining the logic directly with the existing access points/data types. Probably @ame...@google.com could provide some more context.
Adding it through this logic comes with some side effects:
- The dismiss limit will be considered as part of the global limits for the promos considered here (if not part of the SigninLimitsExperiments, which is just an experiment and will probably not launch). Is that ok?
- The flow is tied to a DataType, which at the end, tries to enable and migrate the data. Which does not seem to be the case for the AI search mode.
- Potentially other limitations?
I am just trying to ensure that there is an actual benefit in sharing the code (because it comes with conditional logic for the new access point). If there is value in sharing, that is great! Otherwise it seems like this adding some complexity to an existing flow.
Just making sure I have the complete picture here!
Amelie SchneiderThe reason I looked into the BubbleSignInPromoView as a building block because it provides us already with the the UI to use (everything within the yellow block https://screenshot.googleplex.com/5w27t29G7P97iQG from directly from re-using this view) including handful helpers for the account state and the triggering of the sign-in logic from the button.
I think the benefits we get from this re use are already enough.
I noticed the delegate focuses on the sync data types handing (although the name and the description are more generic).
One approach I can think of, is to produce another delegate for my use case (and extend the existing one).
I agree with reusing the existing `BubbleSignInPromoView`, as visually they are very similar. However, it seems like `BubbleSignInPromoDelegate::OnSignIn` will have to be quite different - assuming that we will need to pass another callback instead of `SelectTypeAndMigrateLocalDataItemsWhenActive`.
I think I like the idea of branching out `OnSignIn()` based on the `access_point_`, e.g. after the call to `SignInFromSingleAccountPromo()`. Another delegate is also an option but I think that for now it is not too big, and the duplicated code also quite minimal when you exclude everything that has to do with data types.
So something like `MaybeMoveDataTypeAfterSignIn()` and `MaybeOpenAIModeAfterSignIn()` ?
Noted for re-using the view - that is definitley a good idea.
I also like the idea of creating a different delegate, which is what I had in mind if we would want to reuse this view. Maybe renaming the existing delegate to be specific to what it is doing? Taking care of the data/dataype basically.
So I would be more inclined to that; as creating a new delegate would be much clearer in my opinion and you would not need to do modifications or add conditional checks in the existing delegate.
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
It turns out that the conditional logic is also clear, and can be simplified a lot compare to my previous patchset. Basically everything after SignInFromSingleAccountPromo should be invokes only if we have a valid data id (PS17). But it should be easy to convert into a delegate, I will produce this approach too (upcoming PS) so we can make the final decision.
Related details
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 2 comments![]( "Open in Gerrit")
Ryan SultanemHi both @ame...@google.com and @rs...@google.com, I see that both of your have worked / reviewed before changes related to the sign-in promo bubbles (for example bookmark_bubble_view which which I used as my blue print).
Whoever of you has the capacity, could you take a look in this (large) CL?
Amelie SchneiderI took a quick look and left high level comments that requires Amelie's input (as she is more familiar with the details of this code).
If possible, it could be clearer to separate the AccessPoint addition into a different CL. That would helpful!
Anthi Orfanou+1 for adding the access point in a separate CL
Done, good point, I've moved the access point to https://chromium-review.git.corp.google.com/c/chromium/src/+/7616929
Done
case signin::SignInPromoType::kSearchAIMode:
// TODO(crbug.com/486858498): Implement prefs for rate limiting.
break;Anthi OrfanouNote; this is part of an experiment related to SigninPromoLimits. Check the start of the function. Which basically makes the dismiss count per data type instead of global (global to all data type promos).
You may find other places where there will be conditional checks based on the feature flag activation or not.
For now I have added this comment in all the places that we could potentially need to adapt for the rate limits. It might be the case than we don't need them all, as this new promo comes with it's own rate limits requirements from the PRD.
I marked all those places because I want to revisit them in the upcoming change where I will focus on the rate limits.
- Amelie Schneider
- Ryan Sultanem
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
case signin_metrics::AccessPoint::kSearchAIModeBubble:
// The AI search mode promo does not relate to to the syncing of any data
// types.
return std::nullopt;Anthi OrfanouThis makes me wonder if this is the right approach - combining the logic directly with the existing access points/data types. Probably @ame...@google.com could provide some more context.
Adding it through this logic comes with some side effects:
- The dismiss limit will be considered as part of the global limits for the promos considered here (if not part of the SigninLimitsExperiments, which is just an experiment and will probably not launch). Is that ok?
- The flow is tied to a DataType, which at the end, tries to enable and migrate the data. Which does not seem to be the case for the AI search mode.
- Potentially other limitations?
I am just trying to ensure that there is an actual benefit in sharing the code (because it comes with conditional logic for the new access point). If there is value in sharing, that is great! Otherwise it seems like this adding some complexity to an existing flow.
Just making sure I have the complete picture here!
Amelie SchneiderThe reason I looked into the BubbleSignInPromoView as a building block because it provides us already with the the UI to use (everything within the yellow block https://screenshot.googleplex.com/5w27t29G7P97iQG from directly from re-using this view) including handful helpers for the account state and the triggering of the sign-in logic from the button.
I think the benefits we get from this re use are already enough.
I noticed the delegate focuses on the sync data types handing (although the name and the description are more generic).
One approach I can think of, is to produce another delegate for my use case (and extend the existing one).
Ryan SultanemI agree with reusing the existing `BubbleSignInPromoView`, as visually they are very similar. However, it seems like `BubbleSignInPromoDelegate::OnSignIn` will have to be quite different - assuming that we will need to pass another callback instead of `SelectTypeAndMigrateLocalDataItemsWhenActive`.
I think I like the idea of branching out `OnSignIn()` based on the `access_point_`, e.g. after the call to `SignInFromSingleAccountPromo()`. Another delegate is also an option but I think that for now it is not too big, and the duplicated code also quite minimal when you exclude everything that has to do with data types.
So something like `MaybeMoveDataTypeAfterSignIn()` and `MaybeOpenAIModeAfterSignIn()` ?
Anthi OrfanouNoted for re-using the view - that is definitley a good idea.
I also like the idea of creating a different delegate, which is what I had in mind if we would want to reuse this view. Maybe renaming the existing delegate to be specific to what it is doing? Taking care of the data/dataype basically.
So I would be more inclined to that; as creating a new delegate would be much clearer in my opinion and you would not need to do modifications or add conditional checks in the existing delegate.
It turns out that the conditional logic is also clear, and can be simplified a lot compare to my previous patchset. Basically everything after SignInFromSingleAccountPromo should be invokes only if we have a valid data id (PS17). But it should be easy to convert into a delegate, I will produce this approach too (upcoming PS) so we can make the final decision.
Lates PS contains the delegate approach.
Related details
- Amelie Schneider
- Ryan Sultanem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
+David for owner's approval for:
chrome/browser/ui/views/autofill/ address_sign_in_promo_view.cc
chrome/browser/ui/views/autofill/ address_sign_in_promo_view.cc
chrome/browser/ui/views/bookmarks/ bookmark_bubble_view.cc
chrome/browser/ui/views/bookmarks/ bookmark_sign_in_promo_bubble_view.cc
chrome/browser/ui/views/search_ai_mode/ signin_promo_manager.h
chrome/browser/ui/views/search_ai_mode/ signin_promo_manager.cc
chrome/browser/ui/views/search_ai_mode/ signin_promo_view.h
chrome/browser/ui/views/search_ai_mode/ signin_promo_view.cc
chrome/browser/ui/views/extensions/ extension_post_install_dialog_view_utils.cc
chrome/browser/ui/views/extensions/ extension_post_install_dialog_view_utils_signin_browsertest.cc
chrome/browser/ui/views/passwords/ password_save_update_view.cc
chrome/app/ chromium_strings.grd
chrome/app/ google_chrome_strings.grd
chrome/browser/ui/ BUILD.gn
Related details
- Amelie Schneider
- David Roger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amelie Schneider (Gerrit)
Amelie Schneider added 9 comments![]( "Open in Gerrit")
Did a first pass, this generally looks good, thanks! :)
One thing I was wondering if it could make sense to make the callsites oblivious to which delegate to use, and to have an optional `DataType` parameter for the `BubbleSigninPromoView` instead. Not sure if this is better though. What do you think?
if (!signin::IsSignInPromo(access_point_)) {Make sure to add the new access point to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/signin/signin_promo_util.cc;drc=17f7265f1db3f3ca266310d68d3a44b1e3361dc4;l=610, else this will apply
// No-op.Will something else happen here at one point?
case SignedInState::kSignInPending:We might want a "Verify it's you" string for the pending state?
if (!base::FeatureList::IsEnabled(switches::kEnableSearchAIModeSigninPromo)) {Could be a CHECK instead, since we should have `ShouldShowSearchAIModeSignInPromo(Profile& profile)` in signin_promo_util.cc which returns false without the flag
And then only call `ShowPromo` if that returns true
// TODO(crbug.com/486858498): Invoke method to check conditions for showingI think this should also be in `ShouldShowSearchAIModeSignInPromo()`
promo_view_->ShowForReason(LocationBarBubbleDelegateView::AUTOMATIC);With this, the button is probably not focused which makes the screen reader not pick it up. I would recommend using `USER_GESTURE`, but might be worth discussing with UX
set_fixed_width(kViewWidth);We should use the layout provider:
```
set_fixed_width(views::LayoutProvider::Get()->GetDistanceMetric(
views::DISTANCE_BUBBLE_PREFERRED_WIDTH));
```
// There no synced data type linked to this sign-in promo.optional nit: `There is no synced...`, or just remove the comment (since this and the other delegate are of equal hierarchy)
Related details
- Anthi Orfanou
- David Roger
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. |