[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
Kelvin Jiang (Gerrit)
Kelvin Jiang added 7 comments![]( "Open in Gerrit")
Thanks Patrick!
Fixed: 7030621Please fix this WARNING reported by Commit Message Validation: Did you intend to reference an old bug id? Please ensure this is a valid Buganiz...
Did you intend to reference an old bug id? Please ensure this is a valid Buganizer ID and not an external system such as Monorail.
ENTERPRISE_POLICY_GETPOLICIES)Patrick Kettnernit: no underscore here -- they should be NAMESPACE_METHODNAME, so ENTERPRISEPOLICY_GETPOLICIES.
Ditto below
Devlin CroninAcknowledged
Doesn't look like this was addressed?
(now it is) @Patrick please resolve comments when the relevant code is updated
#endifnit: `#endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)`
#else // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_CHROMEOS)nit: extra space here
Related details
- Patrick Kettner
Patrick Kettner (Gerrit)
Patrick Kettner added 7 comments![]( "Open in Gerrit")
Please fix this WARNING reported by Commit Message Validation: Did you intend to reference an old bug id? Please ensure this is a valid Buganiz...
Did you intend to reference an old bug id? Please ensure this is a valid Buganizer ID and not an external system such as Monorail.
Done
"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?
Devlin CroninDone
It doesn't look like this one was done?
Sorry about that, I must have screwed up a rebase and lost the change. I moved the definitions to a new target in //chrome/browser/extensions/api/enterprise_policy/BUILD.gn and depend on it from //chrome/browser/extensions/api/BUILD.gn
ENTERPRISE_POLICY_GETPOLICIES)Patrick Kettnernit: no underscore here -- they should be NAMESPACE_METHODNAME, so ENTERPRISEPOLICY_GETPOLICIES.
Ditto below
Devlin CroninAcknowledged
Kelvin JiangDoesn't look like this was addressed?
(now it is) @Patrick please resolve comments when the relevant code is updated
Done
nit: `#endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)`
Done
#else // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_CHROMEOS)Patrick Kettnernit: extra space here
Done
TEST_F(EnterprisePolicyApiUnittest, UploadReport_Success) {Patrick Kettneris there any way we can validate that it uploaded the report?
Devlin CroninI 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?
I updated the test setup to init CloudProfileReportingService and flush it's tasks. WDYT?
Related details
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 1 comment![]( "Open in Gerrit")
- Devlin Cronin
- Kelvin Jiang
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. |
Kelvin Jiang (Gerrit)
Kelvin Jiang added 2 comments![]( "Open in Gerrit")
// The dispatcher forwards calls to the an API function.not sure what this sentence is supposed to say
dispatcher_ = std::make_unique<ExtensionFunctionDispatcher>(profile());I don't see this being used in the test?
Related details
- Devlin Cronin
- Patrick Kettner