Enable Google Update policy display on macOS via Mojo APIs [chromium/src : main]
Aaron Teo (Gerrit)
Aaron Teo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aaron Teo
| Commit-Queue | +1 |
1 comment
Hello, I was browsing the GFB list and found your CL. I will help you run tryjobs as you requested, but I will leave the technical review to the directory owner.
Related details
- Lance Bai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lance Bai (Gerrit)
Lance Bai added 1 comment![]( "Open in Gerrit")
Hello, I was browsing the GFB list and found your CL. I will help you run tryjobs as you requested, but I will leave the technical review to the directory owner.
Hi Aaron,
Thanks for the help. I have updated the patch corresponding to the build failures.
can you trigger the tryjob again, or add win-rel to the check list the tryjobs.
Related details
- Aaron Teo
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. |
Lance Bai (Gerrit)
Lance Bai added 1 comment![]( "Open in Gerrit")
Hi Noah,
I add you as the reviewer, feel free to reassign it if more appropriate.
Related details
- Aaron Teo
- Noah Rose Ledesma
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. |
Aaron Teo (Gerrit)
Aaron Teo added 14 comments![]( "Open in Gerrit")
Lance BaiHello, I was browsing the GFB list and found your CL. I will help you run tryjobs as you requested, but I will leave the technical review to the directory owner.
Hi Aaron,
Thanks for the help. I have updated the patch corresponding to the build failures.
can you trigger the tryjob again, or add win-rel to the check list the tryjobs.
Thank you for updating this patch. Based on the tryjobs results, you also need to update the AUTHORS file to add your name and email address.
https://source.chromium.org/chromium/chromium/src/+/main:AUTHORS
Please also ensure that you have signed the CLA and address my review comments. After you have made the necessary changes, we can run the tryjobs again.
sources += [
"policy/status_provider/updater_status_and_value_provider.cc",
"policy/status_provider/updater_status_and_value_provider.h",
]Are these modifications truly necessary? I see they are already defined in `chrome/browser/policy/BUILD.gn`, and limited to `is_win && is_chrome_branded`
// `app_id` is used to filter per-app policies (e.g., bundle ID on macOS).```suggestion
// |app_id| is used to filter per-app policies (e.g., bundle ID on macOS).
```
const char kTestAppId[] = "com.google.Chrome";
const char kTestJson[] = R"({
"policiesByName": {
"LastCheckPeriod": {
"prevailingSource": "Device Management",
"valuesBySource": {
"Device Management": 120
}
},
"ProxyMode": {
"prevailingSource": "Managed Preferences",
"valuesBySource": {
"Managed Preferences": "fixed_servers"
}
},
"UpdatesSuppressed": {
"prevailingSource": "Device Management",
"valuesBySource": {
"Device Management": {
"StartHour": 1,
"StartMinute": 30,
"Duration": 60
}
}
}
},
"policiesByAppId": {
"com.google.Chrome": {
"Update": {
"prevailingSource": "Device Management",
"valuesBySource": {
"Device Management": 1
}
}
}
}
})";
Perhaps we could use `constexpr`?
// Check global policyPerhaps we could add a `.` (punctuation).
// Check composite policy (UpdatesSuppressed)ditto
// Fetches all the Google Update Policies and state values available through theRedundant?
#if (BUILDFLAG(IS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING)) || \
BUILDFLAG(IS_MAC)Please also add the appropriate owner to the policy-related files.
#include "build/build_config.h"
#include "build/buildflag.h"
#if BUILDFLAG(IS_WIN)
#include <windows.h>
#include <DSRole.h>
#endif
#include <algorithm>
#include <utility>
#include "base/sequence_checker.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/values.h"
#if BUILDFLAG(IS_WIN)
#include "chrome/browser/google/google_update_policy_fetcher_win.h"
#endif
#include "chrome/browser/google/google_update_policy_fetcher_common.h"
#if BUILDFLAG(IS_MAC)
#include "base/apple/foundation_util.h"
#include "base/barrier_closure.h"
#include "chrome/browser/updater/updater.h"
#endifPlease sort the header files according to the style guide, and ensure that any header files you modify or add are required.
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
#endifPerhaps we could add `// if BUILDFLAG(IS_WIN)`?
Regarding the changes to the header file mentioned in the previous comment, it is also recommended to add comments after the endif statement.
Related details
- Lance Bai
- Noah Rose Ledesma
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. |
Aaron Teo (Gerrit)
Aaron Teo added 1 comment![]( "Open in Gerrit")
Perhaps we could add `// if BUILDFLAG(IS_WIN)`?
Regarding the changes to the header file mentioned in the previous comment, it is also recommended to add comments after the endif statement.
Correct a minor flaw in the previous comment.
`// BUILDFLAG(IS_WIN)`
Lance Bai (Gerrit)
Lance Bai added 13 comments![]( "Open in Gerrit")
sources += [
"policy/status_provider/updater_status_and_value_provider.cc",
"policy/status_provider/updater_status_and_value_provider.h",
]Are these modifications truly necessary? I see they are already defined in `chrome/browser/policy/BUILD.gn`, and limited to `is_win && is_chrome_branded`
Done
// `app_id` is used to filter per-app policies (e.g., bundle ID on macOS).```suggestion
// |app_id| is used to filter per-app policies (e.g., bundle ID on macOS).
```
Done
Please delete the extra blank lines here.
Done
const char kTestAppId[] = "com.google.Chrome";Perhaps we could use `constexpr`?
Done
Perhaps we could add a `.` (punctuation).
Done
// Check composite policy (UpdatesSuppressed)Lance Baiditto
Done
// Fetches all the Google Update Policies and state values available through theLance BaiRedundant?
Done
#if (BUILDFLAG(IS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING)) || \
BUILDFLAG(IS_MAC)Please also add the appropriate owner to the policy-related files.
I have run `git cl owners` and added
enterprise-p...@google.com
#include "build/build_config.h"
#include "build/buildflag.h"
#if BUILDFLAG(IS_WIN)
#include <windows.h>
#include <DSRole.h>
#endif
#include <algorithm>
#include <utility>
#include "base/sequence_checker.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/values.h"
#if BUILDFLAG(IS_WIN)
#include "chrome/browser/google/google_update_policy_fetcher_win.h"
#endif
#include "chrome/browser/google/google_update_policy_fetcher_common.h"
#if BUILDFLAG(IS_MAC)
#include "base/apple/foundation_util.h"
#include "base/barrier_closure.h"
#include "chrome/browser/updater/updater.h"
#endifPlease sort the header files according to the style guide, and ensure that any header files you modify or add are required.
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
Done
Aaron TeoPerhaps we could add `// if BUILDFLAG(IS_WIN)`?
Regarding the changes to the header file mentioned in the previous comment, it is also recommended to add comments after the endif statement.
Correct a minor flaw in the previous comment.
`// BUILDFLAG(IS_WIN)`
- Aaron Teo
- Noah Rose Ledesma
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. |
Lance Bai (Gerrit)
Lance Bai added 2 comments![]( "Open in Gerrit")
Lance BaiHello, I was browsing the GFB list and found your CL. I will help you run tryjobs as you requested, but I will leave the technical review to the directory owner.
Aaron TeoHi Aaron,
Thanks for the help. I have updated the patch corresponding to the build failures.
can you trigger the tryjob again, or add win-rel to the check list the tryjobs.
Thank you for updating this patch. Based on the tryjobs results, you also need to update the AUTHORS file to add your name and email address.
https://source.chromium.org/chromium/chromium/src/+/main:AUTHORS
Please also ensure that you have signed the CLA and address my review comments. After you have made the necessary changes, we can run the tryjobs again.
Done
Hi Aaron, thanks for your input, I have
- updated the CL corresponding to your comments.
- signed CLA and added myself into Authors.
- run `git cl owners` and add enterprise-p...@google.com
let me know if I have missed anything.
Related details
- Aaron Teo
- Noah Rose Ledesma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Teo (Gerrit)
Aaron Teo added 4 comments![]( "Open in Gerrit")
Hi Aaron, thanks for your input, I have
- updated the CL corresponding to your comments.
- signed CLA and added myself into Authors.
- run `git cl owners` and add enterprise-p...@google.com
let me know if I have missed anything.
Thank you for updating this CL. There are still a few minor issues. Also, please move `enterprise-p...@google.com` to the Reviewers list and set an Attention Set.
"status_provider/updater_status_and_value_provider.h",This appears to be a duplicate of line 806 above. If this file is necessary, perhaps we should modify the if condition here.
"status_provider/updater_status_and_value_provider.cc",Similar to the previous comment.
#include "chrome/browser/policy/status_provider/updater_status_and_value_provider.h"
#include "build/build_config.h"
#include "build/buildflag.h"
#if BUILDFLAG(IS_WIN)
#include <windows.h>
#include <DSRole.h>
#endif // BUILDFLAG(IS_WIN)
#include <algorithm>
#include <utility>
#if BUILDFLAG(IS_MAC)
#include "base/apple/foundation_util.h"
#include "base/barrier_closure.h"
#endif // BUILDFLAG(IS_MAC)
#include "base/sequence_checker.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/values.h"
#include "chrome/browser/google/google_update_policy_fetcher_common.h"
#if BUILDFLAG(IS_WIN)
#include "chrome/browser/google/google_update_policy_fetcher_win.h"
#endif // BUILDFLAG(IS_WIN)
#include "chrome/browser/policy/chrome_policy_conversions_client.h"
#include "chrome/browser/profiles/profile.h"
#if BUILDFLAG(IS_MAC)
#include "chrome/browser/updater/updater.h"
#endif // BUILDFLAG(IS_MAC)
#include "components/policy/core/browser/policy_conversions.h"
#include "components/policy/core/browser/webui/policy_status_provider.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/web_contents.h"Please reorder the header files as shown below and format them using `git cl format`.
```suggestion
#include "chrome/browser/policy/status_provider/updater_status_and_value_provider.h"
#include <algorithm>
#include <utility>
#include "base/sequence_checker.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/values.h"
#include "build/build_config.h"
#include "build/buildflag.h"
#include "chrome/browser/google/google_update_policy_fetcher_common.h"
#include "chrome/browser/policy/chrome_policy_conversions_client.h"
#include "chrome/browser/profiles/profile.h"
#include "components/policy/core/browser/policy_conversions.h"
#include "components/policy/core/browser/webui/policy_status_provider.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/web_contents.h"
#if BUILDFLAG(IS_MAC)
#include "base/apple/foundation_util.h"
#include "base/barrier_closure.h"
#include "chrome/browser/updater/updater.h"
#endif // BUILDFLAG(IS_MAC)
#if BUILDFLAG(IS_WIN)
#include <windows.h>
#include <DSRole.h>
#include "chrome/browser/google/google_update_policy_fetcher_win.h"
#endif // BUILDFLAG(IS_WIN)
```
Related details
- Lance Bai
- Noah Rose Ledesma
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. |
Aaron Teo (Gerrit)
Aaron Teo added 1 comment![]( "Open in Gerrit")
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
Bug: 507190427```suggestion
Bug: 507190427
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
```
Lance Bai (Gerrit)
Lance Bai added 4 comments![]( "Open in Gerrit")
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
Bug: 507190427```suggestion
Bug: 507190427
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
```
not quite sure if that is caused by my manual editing the comment in the review web ui. I have done another `git cl upload`, hopefully it would be corrected.
"status_provider/updater_status_and_value_provider.h",This appears to be a duplicate of line 806 above. If this file is necessary, perhaps we should modify the if condition here.
I think you are right, I read the gn again, and is_chrome_branded is off by default,
to verify the google policy update feature, some local work around like bellow is required.
```
diff --git a/chrome/browser/policy/BUILD.gn b/chrome/browser/policy/BUILD.gn
index 188a830c0e167..ac5a39e2d49b8 100644
--- a/chrome/browser/policy/BUILD.gn
+++ b/chrome/browser/policy/BUILD.gn
@@ -808,7 +808,7 @@ source_set("policy") {
public += [ "browser_dm_token_storage_mac.h" ]
}
- if (is_chrome_branded && (is_win || is_mac)) {
+ if (is_win || is_mac) {
public += [ "status_provider/updater_status_and_value_provider.h" ]
}
@@ -1137,7 +1137,7 @@ source_set("impl") {
sources += [ "browser_dm_token_storage_mac.mm" ]
}
- if (is_chrome_branded && (is_win || is_mac)) {
+ if (is_win || is_mac) {
sources += [ "status_provider/updater_status_and_value_provider.cc" ]
}
diff --git a/chrome/browser/policy/policy_value_and_status_aggregator.cc b/chrome/browser/policy/policy_value_and_status_aggregator.cc
index ace06ec924272..3e3eee78e6941 100644
--- a/chrome/browser/policy/policy_value_and_status_aggregator.cc
+++ b/chrome/browser/policy/policy_value_and_status_aggregator.cc
@@ -43,11 +43,9 @@
#include "components/prefs/pref_service.h"
#endif // BUILDFLAG(IS_CHROMEOS)
-#if (BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)) && \
- BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
#include "chrome/browser/policy/status_provider/updater_status_and_value_provider.h"
-#endif // (BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)) &&
- // BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#endif // BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
#if BUILDFLAG(ENABLE_EXTENSIONS_CORE)
#include "chrome/browser/policy/cloud/extension_install_policy_service_factory.h"
@@ -150,11 +148,9 @@ const char kDeviceStatusKey[] = "device";
constexpr char kMachineStatusKey[] = "machine";
#endif // !BUILDFLAG(IS_CHROMEOS)
-#if (BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)) && \
- BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
constexpr char kUpdaterStatusKey[] = "updater";
-#endif // (BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)) &&
- // BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#endif // BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
std::unique_ptr<PolicyValueAndStatusAggregator>
PolicyValueAndStatusAggregator::CreateDefaultPolicyValueAndStatusAggregator(
@@ -214,13 +210,11 @@ PolicyValueAndStatusAggregator::CreateDefaultPolicyValueAndStatusAggregator(
#endif // !BUILDFLAG(IS_CHROMEOS)
// Updater policies.
-#if (BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)) && \
- BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
aggregator->AddPolicyStatusAndValueProvider(
kUpdaterStatusKey,
std::make_unique<UpdaterStatusAndValueProvider>(profile));
-#endif // (BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)) &&
- // BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#endif // BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
return aggregator;
}
```
Similar to the previous comment.
Done
#include "chrome/browser/policy/status_provider/updater_status_and_value_provider.h"somehow, `git cl format` does not sort the include headers
I reorder it with the standard headers, then project headers, followed by the platform-specific #if,
let me know if there is anything need to be adjusted
Related details
- Aaron Teo
- Noah Rose Ledesma
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
pma...@chromium.org is from context(chrome/enterprise/gwsq/enterprise-policy-review.gwsq)
Related details
- Aaron Teo
- Noah Rose Ledesma
- Pavol Marko
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. |
Aaron Teo (Gerrit)
Aaron Teo added 4 comments![]( "Open in Gerrit")
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
Bug: 507190427Lance Bai```suggestion
Bug: 507190427
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
```
not quite sure if that is caused by my manual editing the comment in the review web ui. I have done another `git cl upload`, hopefully it would be corrected.
Done
In this case, the file appears to only be compiled on Windows and macOS versions with the Google brand; in other words, this will cause Chromium builds to not compile the file.
The same applies to .cc files.
Was the blank line on line 34 added by `git cl format`?
- Lance Bai
- Noah Rose Ledesma
- Pavol Marko
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. |
Lance Bai (Gerrit)
Lance Bai added 4 comments![]( "Open in Gerrit")
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
Bug: 507190427Lance Bai```suggestion
Bug: 507190427
Change-Id: I38a58db5e28a0d4b72744458660f43d3a72636f0
```
not quite sure if that is caused by my manual editing the comment in the review web ui. I have done another `git cl upload`, hopefully it would be corrected.
Done
"status_provider/updater_status_and_value_provider.h",Done
Was the blank line on line 34 added by `git cl format`?
it is.
Marked as resolved.
Aaron Teo (Gerrit)
Aaron Teo added 2 comments![]( "Open in Gerrit")
"status_provider/updater_status_and_value_provider.h",It seems we still need to consider the Chromium builds?
Lance BaiWas the blank line on line 34 added by `git cl format`?
it is.
Acknowledged
Related details
- Lance Bai
- Noah Rose Ledesma
- Pavol Marko
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. |
Lance Bai (Gerrit)
Lance Bai added 1 comment![]( "Open in Gerrit")
the original BUILD.gn is
```
if (is_win) {
public += [ "browser_dm_token_storage_win.h" ]
if (is_chrome_branded) {
public += [ "status_provider/updater_status_and_value_provider.h" ]
}
}
```updater_status_and_value_provider is intended to be not compiled in the chromium build (since is_chrome_branded is false),
my last update guard the condition with
```
if (is_chrome_branded && (is_win || is_mac)) {
public += [ "status_provider/updater_status_and_value_provider.h" ]
}
```
that should align with the original logic, and extend the feature on the mac platfrom.
let me know if I am missing anything.
Related details
- Aaron Teo
- Noah Rose Ledesma
- Pavol Marko
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. |