[OTP] Define and map OTP filling ActionResultCodes [chromium/src : main]
0 views
Skip to first unread message
Trang Mai (Gerrit)
Jul 3, 2026, 6:16:19 AM (5 days ago) Jul 3
to chromium...@chromium.org, Chromium Metrics Reviews, asvitkine...@chromium.org, browser-comp...@chromium.org, mfoltz+wa...@chromium.org, ipc-securi...@chromium.org
Trang Mai has uploaded the change for review![]( "Open in Gerrit")
Commit message
[OTP] Define and map OTP filling ActionResultCodes
This CL defines Mojo ActionResultCodes for OTP retrieval and filling
errors, registers them in UMA enums.xml, and maps OneTimeTokenRetrievalError
to the corresponding Mojo codes in AttemptOtpFillingTool.
Bug: 502908360
Change-Id: I07d182bf768a87586a069ead182f3432e04989c4
Change diff
Change information
Files:
- M chrome/browser/actor/tools/attempt_otp_filling_tool.cc
- M chrome/browser/actor/tools/attempt_otp_filling_tool_browsertest.cc
- M components/actor/public/mojom/actor_types.mojom
- M tools/metrics/histograms/metadata/actor/enums.xml
Change size: M
Delta: 4 files changed, 89 insertions(+), 4 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Trang Mai (Gerrit)
Jul 6, 2026, 8:51:11 AM (2 days ago) Jul 6
to Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Jeremy Roman
Trang Mai voted Auto-Submit+1![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jul 6, 2026, 11:39:20 AM (2 days ago) Jul 6
to Trang Mai, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Trang Mai
Jeremy Roman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +1 |
1 comment
File components/actor/public/mojom/actor_types.mojom
Line 477, Patchset 13 (Latest):
// Please see the comment above about adding new values.Jeremy Roman . unresolved
should we replicate this comment to the end of this section, too?
Related details
Attention is currently required from:
- Trang Mai
Submit Requirements:
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. |
Trang Mai (Gerrit)
Jul 6, 2026, 3:04:25 PM (2 days ago) Jul 6
to Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Trang Mai voted and added 2 comments![]( "Open in Gerrit")
Votes added by Trang Mai
| Auto-Submit | +1 |
| Commit-Queue | +2 |
2 comments
Patchset-level comments
File components/actor/public/mojom/actor_types.mojom
Line 477, Patchset 13:
// Please see the comment above about adding new values.Jeremy Roman . resolved
should we replicate this comment to the end of this section, too?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Trang Mai (Gerrit)
Jul 6, 2026, 3:05:57 PM (2 days ago) Jul 6
to Min Qin, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Min Qin and Trang Mai
Trang Mai voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Jul 6, 2026, 6:10:09 PM (2 days ago) Jul 6
to Trang Mai, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Trang Mai
Min Qin voted and added 3 comments![]( "Open in Gerrit")
Votes added by Min Qin
| Code-Review | +1 |
3 comments
Patchset-level comments
Min Qin . resolved
lgtm % comments
File chrome/browser/actor/tools/attempt_otp_filling_tool.cc
Line 278, Patchset 14 (Latest):
// TODO(crbug.com/502908360): Add meaningful error message.Min Qin . unresolved
remove this TODO?
File components/actor/public/mojom/actor_types.mojom
Line 483, Patchset 14 (Latest):
kOtpRetrievalTimeout = 1400,Min Qin . unresolved
`kOtpRetrievalTimeout` is defined here and in `enums.xml`, but it isn't mapped anywhere in `AttemptOtpFillingTool::OnOtpRetrieved`.
Previously, `!result.has_value()` defaulted to `kToolTimeout`, but now it defaults to `kOtpRetrievalError` (1401). If a timeout occurs (for example, if the service returns `OneTimeTokenRetrievalError::kSmsOtpBackendTimeout`), it will fall through to the default generic `kOtpRetrievalError`.
If the intent was to map timeout errors to `kOtpRetrievalTimeout`, you should add it to the `switch` statement in `attempt_otp_filling_tool.cc`. If the timeout mapping is planned for a future CL, feel free to ignore this.
Related details
Attention is currently required from:
- Trang Mai
Submit Requirements:
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. |
Trang Mai (Gerrit)
Jul 7, 2026, 3:36:18 AM (yesterday) Jul 7
to Min Qin, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Trang Mai voted and added 2 comments![]( "Open in Gerrit")
Votes added by Trang Mai
| Auto-Submit | +1 |
| Commit-Queue | +2 |
2 comments
File chrome/browser/actor/tools/attempt_otp_filling_tool.cc
Line 278, Patchset 14:
// TODO(crbug.com/502908360): Add meaningful error message.Min Qin . resolved
Trang Mairemove this TODO?
Done. Thank you.
File components/actor/public/mojom/actor_types.mojom
`kOtpRetrievalTimeout` is defined here and in `enums.xml`, but it isn't mapped anywhere in `AttemptOtpFillingTool::OnOtpRetrieved`.
Previously, `!result.has_value()` defaulted to `kToolTimeout`, but now it defaults to `kOtpRetrievalError` (1401). If a timeout occurs (for example, if the service returns `OneTimeTokenRetrievalError::kSmsOtpBackendTimeout`), it will fall through to the default generic `kOtpRetrievalError`.
If the intent was to map timeout errors to `kOtpRetrievalTimeout`, you should add it to the `switch` statement in `attempt_otp_filling_tool.cc`. If the timeout mapping is planned for a future CL, feel free to ignore this.
Trang Mai
You're right, timeout will be planned for a future cl (crbug/530535640).
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Trang Mai (Gerrit)
Jul 7, 2026, 3:57:50 AM (yesterday) Jul 7
to Chromium IPC Reviews, Min Qin, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Chromium IPC Reviews
Trang Mai voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Chromium IPC Reviews
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Jul 7, 2026, 4:02:43 AM (yesterday) Jul 7
to Trang Mai, Chromium IPC Reviews, Camille Lamy, Min Qin, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Camille Lamy
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: cl...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): cl...@chromium.org
Reviewer source(s):
cl...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
Attention is currently required from:
- Camille Lamy
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Jul 7, 2026, 5:16:36 AM (yesterday) Jul 7
to Trang Mai, Chromium IPC Reviews, Min Qin, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Trang Mai
Camille Lamy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Camille Lamy
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Trang Mai
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Trang Mai (Gerrit)
Jul 7, 2026, 5:43:37 AM (yesterday) Jul 7
to Camille Lamy, Chromium IPC Reviews, Min Qin, Jeremy Roman, Ioana Treib, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Camille Lamy
Trang Mai voted Commit-Queue+2![]( "Open in Gerrit")
Attention is currently required from:
- Camille Lamy
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jul 7, 2026, 6:33:53 AM (yesterday) Jul 7
to Trang Mai, Camille Lamy, Chromium IPC Reviews, Min Qin, Jeremy Roman, Ioana Treib, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
15 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/actor/tools/attempt_otp_filling_tool_browsertest.cc
Insertions: 2, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Commit message:
[OTP] Define and map OTP filling ActionResultCodes
This CL defines Mojo ActionResultCodes for OTP retrieval and filling
errors, registers them in UMA enums.xml, and maps
OneTimeTokenRetrievalError to the corresponding Mojo codes in
AttemptOtpFillingTool.
Bug: 502908360
Change-Id: I07d182bf768a87586a069ead182f3432e04989c4
Reviewed-by: Jeremy Roman <jbr...@chromium.org>
Auto-Submit: Trang Mai <tra...@google.com>
Commit-Queue: Trang Mai <tra...@google.com>
Reviewed-by: Min Qin <qin...@chromium.org>
Reviewed-by: Camille Lamy <cl...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1657841}
Files:
- M chrome/browser/actor/tools/attempt_otp_filling_tool.cc
- M chrome/browser/actor/tools/attempt_otp_filling_tool_browsertest.cc
- M components/actor/public/mojom/actor_types.mojom
- M tools/metrics/histograms/metadata/actor/enums.xml
Change size: M
Delta: 4 files changed, 99 insertions(+), 20 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Min Qin, +1 by Jeremy Roman, +1 by Camille Lamy
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
3:06 AM (4 hours ago) 3:06 AM
to Trang Mai, Chromium LUCI CQ, Camille Lamy, Chromium IPC Reviews, Min Qin, Jeremy Roman, Ioana Treib, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Trang Mai
Dominic Battré added 1 comment![]( "Open in Gerrit")
File chrome/browser/actor/tools/attempt_otp_filling_tool.cc
Line 332, Patchset 18 (Latest):
default:
break;Dominic Battré . unresolved
I think that you might regret this if OneTimeTokenRetrievalError grows.
As a minimum I would write
```
message = base::StringPrintF("An error occurred during OTP retrieval: %d",
std::to_underlying(result.error()));
```
Alternatively replace the default cases with a list of all cases so that you learn if a new error code is added.
Related details
Attention is currently required from:
- Trang Mai
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages