[EVP] Add DevTools issues for EVP failures [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
- Christian Biesinger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 1 comment![]( "Open in Gerrit")
// LINT.ThenChange(//content/browser/webid/delegation/email_verification_request.h:EvpRequestStatus)this belongs to EmailVerificationRequestResult, so there should probably be no newline before this but a newline after this
also should there be something similar for the list in Audits.pdl?
and do we really need the mojom version and the .h version? could we just use the mojom version directly in the C++ code?
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 1 comment![]( "Open in Gerrit")
we may want to also add some more details to these issues (e.g. issuer domain) but that can be done later
Yi Gu (Gerrit)
Yi Gu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yi Gu
| Commit-Queue | +1 |
we may want to also add some more details to these issues (e.g. issuer domain) but that can be done later
// LINT.ThenChange(//content/browser/webid/delegation/email_verification_request.h:EvpRequestStatus)this belongs to EmailVerificationRequestResult, so there should probably be no newline before this but a newline after this
also should there be something similar for the list in Audits.pdl?
and do we really need the mojom version and the .h version? could we just use the mojom version directly in the C++ code?
Regarding using mojom as the single source of the enum, as chatted offline, it might be suboptimal to keep the core enum (for metrics and other potential purposes) defined under devtools. We could introduce a new third_party/blink/public/mojom/webid/email_verification_request.mojom as the single source of truth but there's also a concern regarding importing high-level feature specific mojom into low-level ones. Please let me know which works for you. We could also ask the mojom owner for advice if needed.
Related details
- Christian Biesinger
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 2 comments![]( "Open in Gerrit")
Yi Guwe may want to also add some more details to these issues (e.g. issuer domain) but that can be done later
Is it the `kInvalidEmail` in https://chromium-review.git.corp.google.com/c/chromium/src/+/7849254/3/content/browser/webid/delegation/email_verification_request.cc#125?
Just generally I meant more information. For that kInvalidEmail we don't actually have an issuer domain. But e.g. when we have a network error from the issue endpoint, maybe we should show the URL we tried to contact. etc.
But again, we can do that later.
// LINT.ThenChange(//content/browser/webid/delegation/email_verification_request.h:EvpRequestStatus)Yi Guthis belongs to EmailVerificationRequestResult, so there should probably be no newline before this but a newline after this
also should there be something similar for the list in Audits.pdl?
and do we really need the mojom version and the .h version? could we just use the mojom version directly in the C++ code?
Regarding using mojom as the single source of the enum, as chatted offline, it might be suboptimal to keep the core enum (for metrics and other potential purposes) defined under devtools. We could introduce a new third_party/blink/public/mojom/webid/email_verification_request.mojom as the single source of truth but there's also a concern regarding importing high-level feature specific mojom into low-level ones. Please let me know which works for you. We could also ask the mojom owner for advice if needed.
I agree that using devtools as the source of truth is not ideal. I was just hoping that we could find a way to not have 3 copies of these values :/
but I trust your judgement on this, if you want to land as-is.
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 2 comments![]( "Open in Gerrit")
Votes added by Yi Gu
| Commit-Queue | +1 |
PTAL. Thanks!
// LINT.ThenChange(//content/browser/webid/delegation/email_verification_request.h:EvpRequestStatus)Yi Guthis belongs to EmailVerificationRequestResult, so there should probably be no newline before this but a newline after this
also should there be something similar for the list in Audits.pdl?
and do we really need the mojom version and the .h version? could we just use the mojom version directly in the C++ code?
Christian BiesingerRegarding using mojom as the single source of the enum, as chatted offline, it might be suboptimal to keep the core enum (for metrics and other potential purposes) defined under devtools. We could introduce a new third_party/blink/public/mojom/webid/email_verification_request.mojom as the single source of truth but there's also a concern regarding importing high-level feature specific mojom into low-level ones. Please let me know which works for you. We could also ask the mojom owner for advice if needed.
I agree that using devtools as the source of truth is not ideal. I was just hoping that we could find a way to not have 3 copies of these values :/
but I trust your judgement on this, if you want to land as-is.
- Christian Biesinger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger voted and added 3 comments![]( "Open in Gerrit")
Votes added by Christian Biesinger
| Code-Review | +1 |
3 comments
using EvpRequestStatus = blink::mojom::EmailVerificationRequestResult;Have you considered keeping the original name? I tend to find it more difficult to follow code when it renames enums like this.
using EvpRequestStatus = blink::mojom::EmailVerificationRequestResult;I wouldn't use `using` in a header file like this
using EvpRequestStatus = blink::mojom::EmailVerificationRequestResult;same here
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 4 comments![]( "Open in Gerrit")
Votes added by Yi Gu
| Commit-Queue | +1 |
4 comments
+Dave for everything but c/b/webid/. PTAL. Thanks!
using EvpRequestStatus = blink::mojom::EmailVerificationRequestResult;Have you considered keeping the original name? I tend to find it more difficult to follow code when it renames enums like this.
Done
using EvpRequestStatus = blink::mojom::EmailVerificationRequestResult;I wouldn't use `using` in a header file like this
Done
using EvpRequestStatus = blink::mojom::EmailVerificationRequestResult;Yi Gusame here
Done
Related details
- Christian Biesinger
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@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): ort...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Christian Biesinger
- Dave Tapuska
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Ortuno Urquidi
| Code-Review | +1 |
1 comment
- Christian Biesinger
- Dave Tapuska
- Yi Gu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Christian Biesinger
- Yi Gu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Christian Biesinger
- Yi Gu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Sam Goto (Gerrit)
Sam Goto voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Yi Gu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yi Gu (Gerrit)
Yi Gu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yi Gu (Gerrit)
Yi Gu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
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)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/webid/delegation/email_verification_request_unittest.cc
Insertions: 528, Deletions: 80.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/public/mojom/webid/email_verification_request.mojom
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: tools/metrics/histograms/metadata/blink/enums.xml
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/public/devtools_protocol/domains/Audits.pdl
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/browser/webid/delegation/email_verification_request.cc
Insertions: 166, Deletions: 39.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/browser/devtools/devtools_instrumentation.cc
Insertions: 8, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/browser/webid/delegation/email_verification_request.h
Insertions: 7, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
[EVP] Add DevTools issues for EVP failures
The frontend is implemented in crrev.com/c/7850416
NO_IFTTT=There was a missing enum change from ToT.
- M content/browser/devtools/devtools_instrumentation.cc
- M content/browser/webid/delegation/email_verification_request.cc
- M content/browser/webid/delegation/email_verification_request.h
- M content/browser/webid/delegation/email_verification_request_unittest.cc
- M content/browser/webid/delegation/evp_metrics.cc
- M content/browser/webid/delegation/evp_metrics.h
- M content/browser/webid/mappers.cc
- M content/browser/webid/mappers.h
- M content/test/test_render_frame_host.cc
- M content/test/test_render_frame_host.h
- M third_party/blink/public/devtools_protocol/domains/Audits.pdl
- M third_party/blink/public/mojom/BUILD.gn
- M third_party/blink/public/mojom/devtools/inspector_issue.mojom
- A third_party/blink/public/mojom/webid/email_verification_request.mojom
- M third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc
- M tools/metrics/histograms/metadata/blink/enums.xml
Code-Review: +1 by Giovanni Ortuno Urquidi, +1 by Dave Tapuska, +1 by Sam Goto
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |