[AvatarButton] Fix unexpected showing of the Promo state [chromium/src : main]
Ryan Sultanem (Gerrit)
Ryan Sultanem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Commit-Queue | +1 |
1 comment
Hello David, can you please check the following?
Thanks!
Related details
- David Roger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger added 2 comments![]( "Open in Gerrit")
// should not show the promo. Instead clear the state.I don't understand this. Why would we want to disallow showing the promo in this case?
void ShowPromo() {I don't see where this function shows the promo, is this name really accurate?
Related details
- 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. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 2 comments![]( "Open in Gerrit")
// should not show the promo. Instead clear the state.I don't understand this. Why would we want to disallow showing the promo in this case?
I've explained the reasoning more in the commit message.
My thought here is that we wouldn't want to show the promo because we don't have an exact control on when the the states with higher priority will be resolved.
Also when resolving it, we would then present the users with another CTA on the same button, which seems like a less appropriate time.
The initial intention of showing the promo was right after the greeting (and now few seconds after startup with the new promo), the scenario described (which was also problematic in very specific cases) seems like an unexpected case of showing the promo IMO. So my suggestion was to not only fix the crash with this solution, but also prevent showing the promos after resolving any higher priority state.
I don't see where this function shows the promo, is this name really accurate?
Right - reverting to the previous name. It was more accurate indeed.
Related details
- 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. |
David Roger (Gerrit)
David Roger added 1 comment![]( "Open in Gerrit")
// should not show the promo. Instead clear the state.Ryan SultanemI don't understand this. Why would we want to disallow showing the promo in this case?
I've explained the reasoning more in the commit message.
My thought here is that we wouldn't want to show the promo because we don't have an exact control on when the the states with higher priority will be resolved.
Also when resolving it, we would then present the users with another CTA on the same button, which seems like a less appropriate time.The initial intention of showing the promo was right after the greeting (and now few seconds after startup with the new promo), the scenario described (which was also problematic in very specific cases) seems like an unexpected case of showing the promo IMO. So my suggestion was to not only fix the crash with this solution, but also prevent showing the promos after resolving any higher priority state.
I have 2 concerns:
- The greeting is higher priority than the promo, isn't it? I wonder if just excluding all higher priority states is the right thing to do.
- There was a way to get into an inconsistent state. It looks like this CL is not really addressing this problem directly, but rather just working around it. Isn't there a more fundamental bug that we need to fix, so that it should be impossible to be in an inconsistent state to begin with?
Related details
- 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. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 2 comments![]( "Open in Gerrit")
// should not show the promo. Instead clear the state.Ryan SultanemI don't understand this. Why would we want to disallow showing the promo in this case?
David RogerI've explained the reasoning more in the commit message.
My thought here is that we wouldn't want to show the promo because we don't have an exact control on when the the states with higher priority will be resolved.
Also when resolving it, we would then present the users with another CTA on the same button, which seems like a less appropriate time.The initial intention of showing the promo was right after the greeting (and now few seconds after startup with the new promo), the scenario described (which was also problematic in very specific cases) seems like an unexpected case of showing the promo IMO. So my suggestion was to not only fix the crash with this solution, but also prevent showing the promos after resolving any higher priority state.
I have 2 concerns:
- The greeting is higher priority than the promo, isn't it? I wonder if just excluding all higher priority states is the right thing to do.
- There was a way to get into an inconsistent state. It looks like this CL is not really addressing this problem directly, but rather just working around it. Isn't there a more fundamental bug that we need to fix, so that it should be impossible to be in an inconsistent state to begin with?
First one is not really a problem as the enabling of the state is done in two steps; first triggering the promo computation (from the previous state being the Greeting), then requesting an update which would potentially make the current state active. So `new_state` should always be `kPromo` at this point, if we want to show the promo.
Second one is actually slightly problematic you are right - I thought of fixing it by making a more general assumption that the promo should always clear (IMO it still holds, but I will remove from this CL).
The initial problem was that this depended on the order of observer of the Identity Manager triggering a sign out event. If another state provider requests an update as a reaction of the IdentityManager notification (which did not yet reach the PromoStateProvder), then the state would be incorrect.
As a solution now, we just double check that the state is still valid. I will adapt the commit message if you agree with this solution.
Related details
- David Roger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |