[Extensions]: Add enterprise.policy API [chromium/src : main]
Devlin Cronin (Gerrit)
Devlin Cronin added 7 comments![]( "Open in Gerrit")
Thanks, Patrick! This is getting close!
"extensions/api/enterprise_policy/enterprise_policy_api.cc",We shouldn't put these here -- if _absolutely necessary_, we could put them in a target of //chrome/browser/extensions/BUILD.gn, but it'd be much better to have them have their own target, which is then included in //chrome/browser/extensions/api/BUILD.gn. It looks like this is what you had [here](https://crrev.com/c/7030621/29/chrome/browser/extensions/api/BUILD.gn) -- does that still work?
ENTERPRISE_POLICY_GETPOLICIES)nit: no underscore here -- they should be NAMESPACE_METHODNAME, so ENTERPRISEPOLICY_GETPOLICIES.
Ditto below
TEST_F(EnterprisePolicyApiUnittest, UploadReport_Success) {is there any way we can validate that it uploaded the report?
#include "base/barrier_closure.h"nit: not needed
// @return A promise that resolves when the report has successfully uploaded.nit: this is the wrong comment syntax (it's correct above)
Related details
- Kelvin Jiang
- Patrick Kettner
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. |
Patrick Kettner (Gerrit)
Patrick Kettner added 6 comments![]( "Open in Gerrit")
"extensions/api/enterprise_policy/enterprise_policy_api.cc",We shouldn't put these here -- if _absolutely necessary_, we could put them in a target of //chrome/browser/extensions/BUILD.gn, but it'd be much better to have them have their own target, which is then included in //chrome/browser/extensions/api/BUILD.gn. It looks like this is what you had [here](https://crrev.com/c/7030621/29/chrome/browser/extensions/api/BUILD.gn) -- does that still work?
Done
nit: no underscore here -- they should be NAMESPACE_METHODNAME, so ENTERPRISEPOLICY_GETPOLICIES.
Ditto below
Acknowledged
TEST_F(EnterprisePolicyApiUnittest, UploadReport_Success) {is there any way we can validate that it uploaded the report?
I dont think so (at least as far as I can tell). I am reusing the existing `UploadFullReport`, and as far as I am able to reason it only takes a completion closure and doesn't pass back a status for the actual upload. It "succeeds" as long as it triggers the process. I assume that adding anything would require work on the server side?
#include "base/barrier_closure.h"Patrick Kettnernit: not needed
Done
// @return A promise that resolves when the report has successfully uploaded.nit: this is the wrong comment syntax (it's correct above)
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 4 comments![]( "Open in Gerrit")
Thanks, Patrick! But it looks like some of these comments weren't addressed?
"extensions/api/enterprise_policy/enterprise_policy_api.cc",Patrick KettnerWe shouldn't put these here -- if _absolutely necessary_, we could put them in a target of //chrome/browser/extensions/BUILD.gn, but it'd be much better to have them have their own target, which is then included in //chrome/browser/extensions/api/BUILD.gn. It looks like this is what you had [here](https://crrev.com/c/7030621/29/chrome/browser/extensions/api/BUILD.gn) -- does that still work?
Done
It doesn't look like this one was done?
ENTERPRISE_POLICY_GETPOLICIES)Patrick Kettnernit: no underscore here -- they should be NAMESPACE_METHODNAME, so ENTERPRISEPOLICY_GETPOLICIES.
Ditto below
Acknowledged
Doesn't look like this was addressed?
TEST_F(EnterprisePolicyApiUnittest, UploadReport_Success) {Patrick Kettneris there any way we can validate that it uploaded the report?
I dont think so (at least as far as I can tell). I am reusing the existing `UploadFullReport`, and as far as I am able to reason it only takes a completion closure and doesn't pass back a status for the actual upload. It "succeeds" as long as it triggers the process. I assume that adding anything would require work on the server side?
I'm not so much suggesting we test the full end-to-end flow with a "real" report upload, but rather that we check that we (at least attempted to) trigger the report upload. Right now, if we replaced the body of the function with just "RespondNow(NoArguments())", this test would still pass.
It looks like a few unit tests use the [last upload time](https://source.chromium.org/chromium/chromium/src/+/main:components/enterprise/browser/reporting/common_pref_names.cc;l=20;drc=5cfe525024bc42f5433fff03ee62998590268c2b) preference to check that a report was uploaded; perhaps that would work?
Related details
- Kelvin Jiang
- Patrick Kettner