[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
Kelvin Jiang (Gerrit)
Kelvin Jiang added 1 comment![]( "Open in Gerrit")
Hey Patrick could you do a fresh pull and resolve merge conflicts? then let's try to get a dry run to pass before continuing with review
Devlin Cronin (Gerrit)
Devlin Cronin added 11 comments![]( "Open in Gerrit")
Thanks, Patrick!
For CLs, please also make sure you do a "self-review" when uploading a new patch set to make sure all comments are addressed and all changes are intentional and necessary -- that can help catch some of these cases of unintentional changes or bad merge resolutions.
"guest_view/app_view/chrome_app_view_guest_delegate.cc",why these changes? They seem very unrelated to this work
"api/enterprise_policy/enterprise_policy_api.cc",as mentioned [here](https://crrev.com/c/7030621/39..51/chrome/browser/BUILD.gn#b7354), we should only put these in //c/b/extensions/BUILD.gn if they can't go in the separate BUILD.gn in the API dir. Does that not work for some reason?
"//services/network/public/cpp",is this needed?
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?
Patrick KettnerI'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?
This looks better; thanks!
enterprise_reporting::kLastUploadTimestamp) != base::Time();we should verify that it *is* set to base::Time() before the call; otherwise, this would pass even if the function didn't do anything
TEST_F(EnterprisePolicyApiUnittest, UploadReport_IncognitoProfile) {what is the expected behavior here? Should it upload the report for the main profile, the incognito profile, something else? As this test is today, it's very difficult to tell what should happen
TEST_F(EnterprisePolicyApiUnittest, Reload_Success) {similar to the comment above, is there some way we can test that this function properly triggers a reload?
[permissions = "enterprisePolicy"]this isn't an attribute we use -- did you usee this pattern used elsewhere?
// |Returns| A promise that resolves when the report has successfully uploaded.need a colon after |Returns|
`|Returns|: A promise...`
(for all of these)
// |Returns| A promise that resolves when the report has successfully uploaded.wrap at 80 char
Related details
- 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 13 comments![]( "Open in Gerrit")
Thanks, Patrick!
For CLs, please also make sure you do a "self-review" when uploading a new patch set to make sure all comments are addressed and all changes are intentional and necessary -- that can help catch some of these cases of unintentional changes or bad merge resolutions.
Acknowledged
"guest_view/app_view/chrome_app_view_guest_delegate.cc",why these changes? They seem very unrelated to this work
Unintentional formatting change leftover from a bad merge/rebase. Reverted
as mentioned [here](https://crrev.com/c/7030621/39..51/chrome/browser/BUILD.gn#b7354), we should only put these in //c/b/extensions/BUILD.gn if they can't go in the separate BUILD.gn in the API dir. Does that not work for some reason?
Done. moved the over to a dedicated source_set in the specific BUILD.gn within the API dir
"//services/network/public/cpp",Patrick Kettneris this needed?
Nope, leftover from an earlier iteration. Removed
// The dispatcher forwards calls to the an API function.not sure what this sentence is supposed to say
removed
dispatcher_ = std::make_unique<ExtensionFunctionDispatcher>(profile());I don't see this being used in the test?
The dispatcher was no longer needed since api_test_utils::RunFunction creates its own. Removed
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?
Patrick KettnerI'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?
Devlin CroninI updated the test setup to init CloudProfileReportingService and flush it's tasks. WDYT?
This looks better; thanks!
Done
enterprise_reporting::kLastUploadTimestamp) != base::Time();we should verify that it *is* set to base::Time() before the call; otherwise, this would pass even if the function didn't do anything
Done
TEST_F(EnterprisePolicyApiUnittest, UploadReport_IncognitoProfile) {what is the expected behavior here? Should it upload the report for the main profile, the incognito profile, something else? As this test is today, it's very difficult to tell what should happen
I've updated the test to explicitly assert that CloudProfileReportingServiceFactory returns null ptr in incognito. This clarifies why the report scheduler path safely early-returns and is skipped for incognito. (fun fact - b/513868285)
similar to the comment above, is there some way we can test that this function properly triggers a reload?
done. refactored the test to use a mocked PolicyService
this isn't an attribute we use -- did you usee this pattern used elsewhere?
Done
// |Returns| A promise that resolves when the report has successfully uploaded.need a colon after |Returns|
`|Returns|: A promise...`
(for all of these)
Done
// |Returns| A promise that resolves when the report has successfully uploaded.Patrick Kettnerwrap at 80 char
Done
Related details
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
1 comment
Related details
- Devlin Cronin
- 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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 13 comments![]( "Open in Gerrit")
Thanks, Patrick!
[submodule "third_party/libphonenumber/src"]gitmodules should not be changed in this CL, most likely
":enterprise_policy",nit: put local references above // references
"//chrome/browser/policy:policy",nit: foo:foo is the same as foo; prefer just //chrome/browser/policy
return RespondNow(NoArguments());optional: should this be an error, since no report is actually submitted? (Up to you on what you think the behavior should be)
FakeReportUploader() : ReportUploader(nullptr, 0) {}nit: please document anonymous parameters:
```
(/*client=*/nullptr, /*maximum_number_of_retries=*/0)
```
::enterprise_reporting::ReportUploader::ReportCallback callback)nit: you probably don't need this prefix, since its inherited from the super class. Ditto for line 53
#else // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_CHROMEOS)Only uploadReport() is unsupported, right? Can we have the tests for GetPolicies() and Reload() across all platforms?
// Verify the timestamp is unset before the call.
ASSERT_EQ(base::Time(), profile()->GetPrefs()->GetTime(
enterprise_reporting::kLastUploadTimestamp));nit: indentation off
EXPECT_TRUE(base::test::RunUntil([&]() {is this RunUntil needed? The function should only finish once the report is uploaded, and RunFunction() will wait for that to happen.
TEST_F(EnterprisePolicyApiUnittest, UploadReport_IncognitoProfile) {Patrick Kettnerwhat is the expected behavior here? Should it upload the report for the main profile, the incognito profile, something else? As this test is today, it's very difficult to tell what should happen
I've updated the test to explicitly assert that CloudProfileReportingServiceFactory returns null ptr in incognito. This clarifies why the report scheduler path safely early-returns and is skipped for incognito. (fun fact - b/513868285)
Ack; thanks, Patrick!
Two questions:
1) Are we sure we don't want an error here? It seems odd to indicate to the extension that the upload succeeded when no upload took place.
2) Can we verify a report _wasn't_ uploaded by checking kLastUploadTimestamp is base::Time()?
Related details
- Patrick Kettner