[EVP] Only trigger strike after user explicitly declines the permission [chromium/src : main]
Yi Gu (Gerrit)
Yi Gu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yi Gu
| Commit-Queue | +1 |
1 comment
Related details
- Mohamed Amir Yosef
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
Mohamed Amir Yosef voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mohamed Amir Yosef
| Code-Review | +1 |
3 comments
if (result == EmailVerificationResult::kDismissed) {Since `EXPECT_EQ(result, EmailVerificationResult::kIgnored)` ensures `result` won't be `kDismissed` (otherwise the test fails), this `if` block acts as dead code and can safely be removed.
kDismissed = 1,Please fix this WARNING reported by autoreview issue finding: Consider renaming `kDismissed` to `kDeclined`.
"Dismissed" is generally interpreted as closing or ignoring a dialog without taking explicit action (which corresponds to `kIgnored` here). Even the test names reflect this terminology: `VerificationDismissed` simulates `kIgnored`, while `DeclineAddsStrike` simulates `kDismissed`.
Renaming this would clarify that this enum value represents an explicit user decline and better align with both the commit message and the UI telemetry (`EvpPermissionUiStatus::kDeclined`).
Related details
- Yi Gu
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. |
Sam Goto (Gerrit)
Sam Goto added 3 comments![]( "Open in Gerrit")
enum class EmailVerificationResult {EmailVerificationPromptResult?
Related details
- Yi Gu
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. |
Yi Gu (Gerrit)
Yi Gu voted and added 5 comments![]( "Open in Gerrit")
Votes added by Yi Gu
| Commit-Queue | +1 |
5 comments
if (result == EmailVerificationResult::kDismissed) {Since `EXPECT_EQ(result, EmailVerificationResult::kIgnored)` ensures `result` won't be `kDismissed` (otherwise the test fails), this `if` block acts as dead code and can safely be removed.
Done
kDismissed = 1,Yi GukNotNow?
I renamed it to Declined per Mohamed's suggestion. "NotNow" is string centric and the essence of the action is "user declining the prompt". Let me know if you feel strongly about using "NotNow".
enum class EmailVerificationResult {Yi GuEmailVerificationPromptResult?
Good point. Although I used EmailVerificationPermissionUiResult instead. "prompt" is a bit generic IMO. e.g. the toast may also be called "prompt" since it has buttons taking actions (especially for opt-in users the toast is the only UI in EVP).
kDismissed = 1,Please fix this WARNING reported by autoreview issue finding: Consider renaming `kDismissed` to `kDeclined`.
"Dismissed" is generally interpreted as closing or ignoring a dialog without taking explicit action (which corresponds to `kIgnored` here). Even the test names reflect this terminology: `VerificationDismissed` simulates `kIgnored`, while `DeclineAddsStrike` simulates `kDismissed`.
Renaming this would clarify that this enum value represents an explicit user decline and better align with both the commit message and the UI telemetry (`EvpPermissionUiStatus::kDeclined`).
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |